Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes for issues blake502#103 #129

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Fixes for issues blake502#103 #129

wants to merge 8 commits into from

Conversation

DazeChr
Copy link

@DazeChr DazeChr commented Dec 27, 2024

Fixes the "Balatro.exe" deletion when you don't create a build and add the version number as well. Based on the code from user @uday-sudo.

@DazeChr
Copy link
Author

DazeChr commented Dec 27, 2024

Edit: "and Fix for issue #124"

@DazeChr DazeChr closed this Dec 27, 2024
@DazeChr DazeChr reopened this Dec 27, 2024
@blake502
Copy link
Owner

I'll need some time to review the changes, but looks good at first glance.

@DazeChr
Copy link
Author

DazeChr commented Dec 28, 2024

Thanks for checking them!

@OmiKat
Copy link

OmiKat commented Jan 4, 2025

@DazeChr Don't you think taking @uday-sudo code removes his attribution from the project.

You could have kept his commits and made a pull request over them.

@uday-sudo
Copy link

@DazeChr I think it is wrong of you to take my code this way, you could have taken my PR and pushed your changes over them. You have basically copied my PR and that is all that is meaningful in this PR.

Also I see you have added x64 along with Any CPU which was not required and unnecessary.

Also I think you have changed the end of line control character, which increased the patch size even though the changes were very small in those files.

I do not mean to be mean, but you should make a new PR with your changes and not piggyback off my contributions.

@DazeChr
Copy link
Author

DazeChr commented Jan 4, 2025

Hey, sorry for the confusion this made, and sorry for if I came off as rude for what I did. I mostly forked it and fixed issue #124 myself and for myself. I was unaware of a fix before, then I saw it and I saw your others fix, so I added it.

That's the reason I mentioned you, so you can have credit for your contributions made. If you want, I can close this PR, I have no issue with doing that.

@uday-sudo
Copy link

I do not want you to close this PR, I want you to contribute correctly.

If you want to make changes over my changes do it by pushing them over my commits, not by copy pasting my code and committing yourself.

If you want to publish your changes for #124 do it, I don't have a problem with that.

Instead of closing this PR remove my changes by editing the PR.

I just want my contributions to be recorded as mine.

@DazeChr
Copy link
Author

DazeChr commented Jan 4, 2025

Oh sure, I'll do that. I want to add as well because I just read my comment and realized it. I meant issue #103. I fixed that myself and will keep. I'll revert changes from issue #124.

Again, sorry if this seemed with ill intentions, thanks for the help!

Also, sorry for asking since I'm still new to GitHub, by pushing my changes over yours, do you mean in like forking your repo or in another way?

@DazeChr DazeChr changed the title Fixes for issues blake502#103 and blake502#103 Fixes for issues blake502#103 Jan 4, 2025
@uday-sudo
Copy link

I will help you contribute better for next time @DazeChr

Firstly, if you want to make changes over somebody else's changes: https://gist.github.com/wtbarnes/56b942641d314522094d312bbaf33a81
Also you only need to do this if your changes are dependent on their changes, which in this case they weren't so you could have made a PR with just you changes.

Secondly, it seems like you have changed the end of line control character, which increased the patch size even though the changes were very small in those files. For example the diff for View.cs says you changes the whole file. This will make it hard for reviewers to review your changes and also make it so the git blame doesn't accurately reflect the changes. Which on closer inspection doesn't seem to be the issue as locally it is showing me the correct diff. Leave it for now I guess

Thirdly, If somebody has submitted a solution for an issue, unless your solution is better you don't really need to submit code for the same as it will cause difficulty and duplicate work for the reviewers.

Fourthly, The other files that you have changed have nothing thing to do with the issue #103, so you could elaborate on how those changes benefit the project here. It will make it easier for the reviewers to review you code.

In the starting this could be overwhelming, don't take this the wrong way and don't stop contributing.

@DazeChr
Copy link
Author

DazeChr commented Jan 5, 2025

Omg, I didn't really expect this kind of response. Thank you for taking your time to answer and even providing advice, This is really helpful and makes things easier to understand.

I had some issues with VS, that's why it's a mess and I didn't really check the other PR, that's why I fixed it myself.

Again, thank you for taking the time to help me with this! Really appreciate it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants