lethal-guitar / RigelEngine

A modern re-implementation of the classic DOS game Duke Nukem II
GNU General Public License v2.0
914 stars 60 forks source link

Correct spelling in the code: "dieing" -> "dying" #872

Closed ghost closed 2 years ago

ghost commented 2 years ago

why isn't merge , i replaced every words in player.cpp and player.hpp

lethal-guitar commented 2 years ago

@GAbdulWahab ok first of all, I need some time to look at this. It's early morning in my timezone, I have just woken up 30 minutes ago. Have some patience, this is a hobby project that I'm working on in my free time. I was actually just about to write a comment to you. But it's pretty normal for open source maintainers to not react immediately to everything. We have lives outside our projects, too, you know.

Now, back to topic.

Thanks for taking the time to work on this. But some more work is needed before I can merge your contribution:

ghost commented 2 years ago

@lethal-guitar thanks sir for helping me, I appreciate you

lethal-guitar commented 2 years ago

@GAbdulWahab I see that you've fixed the casing, thanks 👍🏻. One step closer!

You still currently have two pull requests open though, and I see that you've pushed changes to both of them. Which one of the two would you like to continue working with? Please decide on one of them, and close the other one. I'd suggest keeping this one open since this is where we're having a conversation but it's really up to you. The important part is just to consolidate everything into a single PR.

Also, your changes still have compilation errors right now - see AppVeyor CI log for example. Maybe these errors are due to your changes being distributed across two PRs, but I haven't checked too closely since it's much easier for me to review the changes when everything is in one place. Again, I'd suggest that you set up a local build environment on your machine (I've posted a link to the instructions above), and try to compile the project locally. Then push your code again once it compiles without errors.

Feel free to ask questions if you run into any issues with that or get stuck.

ghost commented 2 years ago

@lethal-guitar Hi sir I hope you fine, sir I'm not making a clone to this project on my pc ... I make replace to every word on Github I go to every file (.cpp and .hpp) and replace and make a PR for every one

lethal-guitar commented 2 years ago

@GAbdulWahab Well, if you want to make a contribution that changes code, you really need to clone the repository locally on your PC, no way around that. Editing files directly in the GitHub web interface is great for editing documentation files or things of that nature, but anything with strict syntax and semantics (like code) should really only be edited locally. Let me explain why.

It's a generally accepted best practice in software development nowadays that you want the code in the repository's main branch to always be in a state where it's usable. This means the code should compile without errors, and pass all unit tests (and perhaps other types of tests, like integration tests, acceptance tests etc.) This way, you can be sure that anybody who clones the repo in order to start working on the code can start from a known good state. And ideally, you could always create a release from the code and ship it to your customers.

To ensure that this is the case, continuous integration systems are used, also known as CI systems. The majority of open source projects nowadays have something like this, RigelEngine is no exception. Every time someone creates a pull request for RigelEngine, the changes that they made are automatically tested to see if the code compiles and tests pass. Before merging a PR, all of these automated checks must be successful - this is often called "green", since success is indicated by a green color usually. I will never merge a PR which isn't green, and in fact I cannot even merge it, because the checks are required (I've configured it that way). So because of that, you absolutely must have all your changes in a single PR, and that's only possible when you clone the code locally. Otherwise, if you have one PR that changes player.hpp and another one that changes player.cpp, then the code will not compile for either of these PRs, and neither of them can be merged.

But even if we ignore the CI system, it still makes the most sense to edit the code locally. In C++, it's very easy to forgot to change something or make a simple mistake like a missing ; or } or similar. This is totally normal, it happens to the most experienced developers. As soon as you try to compile the code, you will immediately see these errors, and you'll be able to correct them. But if you change the code in the GitHub web interface, you have no way of compiling the code. Thus, you cannot know if the changes you've made are actually valid or not. You will find out as soon as you make a PR and the CI system checks the code. But that takes a bit of time, and you need a few clicks to get to the actual error message. If you compile the code locally, you get feedback immediately.

So, to summarize, if you want to contribute to this project, you really need to set up a local dev environment so that you can put all changes into one PR. I still appreciate your contribution, and your time investment, but I'm afraid I can't accept it in its current state.

I realize that this might not be 100% clear, so as an improvement for the future, I will make sure that this is explained in CONTRIBUTING.md and I will add a pull request template which also explains the minimum requirements for a PR.

ghost commented 2 years ago

@lethal-guitar Hello sir, I hope you are fine. I would like to thank you a lot for your wonderful advice and thank you once again for allowing me to participate despite my lack of experience in participating in open projects. I hope you accept the changes I made. I encountered some problems, but I tried to fix them I want to see what I did here #875

lethal-guitar commented 2 years ago

@GAbdulWahab great, that looks much better! 👍🏻 Let's continue on your new PR then, I'll close this one.