-
Notifications
You must be signed in to change notification settings - Fork 72
Add CONTRIBUTING document #548
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
base: main
Are you sure you want to change the base?
Conversation
suggestion (for CONTRIBUTE document, possibly different wording): This is generally good practice for projects with LTS (long term support) releases, where development continues but older releases are still supported. (It also helps not to end in a mess.) |
Should I write a draft for the PR and Changelog sections? |
We can keep discussing in your change log PR: #523 I am currently unable to keep up with reviews because of the volume. |
071f464
to
6b32224
Compare
All comments addressed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Contains unrelated commits
CONTRIBUTING.md
Outdated
|
||
### Scope of code changes | ||
|
||
Code edits only touch the lines of code that serve the intended goal of the change. Big refactors should not be combined with logical changes, because these can become very difficult to review. If a change requires a refactor, create a commit for the refactor before (or after) creating a commit for the change. A Pull Request can contain multiple commits and can be merged with **Rebase and Merge** if these commits are meant to be preserved on the main branch. Otherwise, the regular method of merging is **Squash and Merge**. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the regular method of merging is **Squash and Merge**.
I think we should put this up to a vote - I still think Squash
is meant as a last resort for ugly PRs, but shouldn't be the default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having Rebase and Merge as default is not a logical default, because it does not cater to the most common types of Pull Requests and will cause more work and delays.
A Pull Request that was not designed to preserve multiple commits will then need to be manually squashed or cleaned up before merge, instead of doing it with the click of a button in GitHub. It means the original author needs to hand craft the cleanup, before the maintainer can approve and submit it.
Pull Requests with multiple commits to be preserved are not the norm. The norm are Pull Requests with a single commit or with chaos commits.
Pull Requests that have their review edits squashed into the first commit are also discouraged, because the reviewer will not be able to see diff from one edit to another. Having incremental commits makes things easier to review when the reviewer asked for changes.
One can use !fixup commits when commits needs to be preserved, otherwise just commit freestyle if the pull request is meant to be merged with squash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be fair the document already states that PRs prepared to be merged as multiple commits can be so that only really leaves squashing for PRs that aren't prepared that way. PR merge style is more of a repository maintainence thing than a contributor issue really other than if they want to worry about preserving their commits or not.
Maybe reword the last sentence to "Otherwise, method of merging will be Squash and Merge.
Really we are only ruling out the github default of creating an additional merge commit and allowing none linear history.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe reword the last sentence to "Otherwise, method of merging will be Squash and Merge."
Fixed
CONTRIBUTING.md
Outdated
|
||
### Style of code changes | ||
|
||
Code edits should fit the nearby code in ways that the code style reads consistent, unless the original code style is bad. The original game code uses c++98, or a deviation thereof, and is simple to read. Prefer to not use new fancy language features where not absolutely necessary to get a particular change done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is temporary until VC6 is dropped? Because otherwise I'd certainly prefer auto
and ranged-based for-loops
. Maybe just mention that it must compile with VC6?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not yet have code style guidelines.
Range-based for loops are fine.
For auto, we will go into the direction of google style guide:
The fundamental rule is: use type deduction only to make the code clearer or safer, and do not use it merely to avoid the inconvenience of writing an explicit type. When judging whether the code is clearer, keep in mind that your readers are not necessarily on your team, or familiar with your project, so types that you and your reviewer experience as unnecessary clutter will very often provide useful information to others. For example, you can assume that the return type of make_unique() is obvious, but the return type of MyWidgetFactory() probably isn't.
https://google.github.io/styleguide/cppguide.html#Type_deduction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When did we decide on that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Core Guidelines say: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-auto
ES.11: Use auto to avoid redundant repetition of type names
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove the word "fancy" from describing language features as it implies a value judgement on their suitability and just leave it as something like "Prefer not to use newer language features unless required to implement the desired change."
Regarding the guidelines you both refer to, I read them as pretty much saying the same thing in different ways.
auto widget = MyWidgetFactory<WidgetType>(); // good
WidgetType widget = MyWidgetFactory<WidgetType>(); // acceptable
auto widget = MyWidgetFactory(); // bad
WidgetType widget = MyWidgetFactory(); // good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I also appended additional sentence "Prefer to use newer language features when they are considerably more robust or make the code easier to understand or maintain."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove the word "fancy" from describing language features as it implies a value judgement on their suitability and just leave it as something like "Prefer not to use newer language features unless required to implement the desired change."
Regarding the guidelines you both refer to, I read them as pretty much saying the same thing in different ways.
auto widget = MyWidgetFactory<WidgetType>(); // good WidgetType widget = MyWidgetFactory<WidgetType>(); // acceptable auto widget = MyWidgetFactory(); // bad WidgetType widget = MyWidgetFactory(); // good
That is in not what the standard guidelines say. As a matter of fact what they say is the complete opposite. This is the example they give as a reference for good usage of auto
auto p = v.begin(); // vector<DataRecord>::iterator
auto z1 = v[3]; // makes copy of DataRecord
auto& z2 = v[3]; // avoids copy
const auto& z3 = v[3]; // const and avoids copy
auto h = t.future();
auto q = make_unique<int[]>(s);
auto f = [](int x) { return x + 10; };
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite sure how you think that is opposite, those examples are mostly orthognal or in agreement. The make_unique and lambda examples clearly follow my line of reasoning in that they already express the type in the statement.
The container examples I don't agree with apart from begin(), but that is a separate advocated use of auto... to allow easy swapping out of standard container types, not the use case of avoiding repeating yourself on types that was raised in this discussion.
* [GITHUB] | ||
* [LINUX] | ||
|
||
If the Pull Request is meant to be merged with rebase, then a note for **Merge with Rebase** should be added to the top of the text body, to help identify the correct merge action when it is ready for merge. All commits of the Pull Request need to be properly named and need the number of the Pull Request added as a suffix in parentheses. Example: **(#333)**. All commits need to be able to compile on their own without dependencies in newer commits of the same Pull Request. Prefer to create changes for **Squash and Merge**, as this will simplify things. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as with the default "Squash and merge". I'd prefer putting this up to a vote. When using Rebase and merge
PR numbers inside the commit message are a headache
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm personally not a fan of adding the PR number to the commit messages or generally referring to github or this repo specific things in them as if cloned or forked they won't lead back to the relevant items at all and will end up meaningless. Referring to issues in PRs and PRs in issues is fine as they are (mostly) locked to the repo in question and won't be split from each other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if cloned or forked they won't lead back to the relevant items at all
This is a reasonable argument against this practice. feliwir is also not a fan of this. So do we prefer to not do that then and abandon this practice now? Note that "Squash and Merge" will automatically place this number into the commit title form. We would have to manually remove it before each "Squash and Merge".
Note that the link in the commit makes it easier to look up the Pull Request or find a textual change log entry that is identified with the pull request number. It also groups related commits together in the commit history. We would partly lose that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm in favour of dropping the practice personally, I appreciate that it makes it easier to follow a commit back to the PR it was merged in, butfor me technical limitation that it only works while the repo is here in github and may point elsewhere unrelated when cloned or if ever moved is enough for me. The fact that its slightly onerous for multi commit PRs could be an acceptable tradeoff otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if cloned or forked they won't lead back to the relevant items at all
If a repository is forked within GitHub, the pull request in the original branch remains linked. The real risk of breaking these references arises if the repository is migrated away from GitHub. In such cases, open issues would need to be transferred, while historical issues and PRs could be lost entirely. Given the broader impact of such a move, this should not be a deciding factor against adopting a particular practice.
I found this practice to be helpful when I was looking for introduced bugs that caused replay mismatches.
A relevant consideration is whether referencing PR numbers in commit messages is a common convention. GitHub integrates this functionality well, and large organizations like Microsoft and IBM follow this practice.
Overall, referencing PR numbers in commits appears to be a valuable practice. I support continuing to use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Short (72 chars or less) summary More detailed explanatory text. Wrap it to 72 characters. The blank line separating the summary from the body is critical (unless you omit the body entirely). Write your commit message in the imperative: "Fix bug" and not "Fixed bug" or "Fixes bug." This convention matches up with commit messages generated by commands like git merge and git revert. Further paragraphs come after blank lines. - Bullet points are okay, too. - Typically a hyphen or asterisk is used for the bullet, followed by a single space. Use a hanging indent.
Sourced from https://gist.github.com/robertpainsi/b632364184e70900af4ab688decf6f53
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How often is someone going to want to go from a commit to a PR though in practice? I don't think we should be storing any github specific stuff in the commit history. Linking a PR and an Issue together I agree is good practice and is all stored within GitHub itself. Github forks only work if the fork remains in the network as I understand it, if we were to break with the EA repo as a parent or someone hard forked it on github, the problem still stands that they either point no where or end up pointing at wrong things. If GitHub at some point pulls a Source Forge and becomes a bad actor and we want to pull away from it, the commit history then has a bunch of useless stuff in it.
The commit title to pull is a valuable association in regards to writing and maintaining change logs. I know this because it helped me for the change logs in the other repository GeneralsGamePatch, which does the same thing in commit titles. It also helps to see which commits belong together in a bundle if they were merged from one Pull.
I recommend to view these numbers not just as GitHub associated id's, but id's in general that can be used for other identification purposes, regardless of the commit hash. A commit hash can change on a rebase, but the id in a title does not change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If its helpful to automated tooling for generating change logs then I'd agree its useful, but if it isn't or the same association can be made other ways with automated tooling then I'd stick to my original view.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Short (72 chars or less) summary More detailed explanatory text. Wrap it to 72 characters. The blank line separating the summary from the body is critical (unless you omit the body entirely). Write your commit message in the imperative: "Fix bug" and not "Fixed bug" or "Fixes bug." This convention matches up with commit messages generated by commands like git merge and git revert. Further paragraphs come after blank lines. - Bullet points are okay, too. - Typically a hyphen or asterisk is used for the bullet, followed by a single space. Use a hanging indent.
Sourced from https://gist.github.com/robertpainsi/b632364184e70900af4ab688decf6f53
yes, but this just explains how git commit works since not everyone seems to understand that it's possible to write as much in there as you want
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If its helpful to automated tooling for generating change logs then I'd agree its useful
It is for manual review purposes. I can see the immutable id in a commit and then use that to find associated resources that correspond to that very id. When the id is not in the commit title, then I need to search for it in other ways, which can take more effort.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The document looks very good, maybe it would be worth adding a paragraph about the review process, something like:
Review process
After submitting a Pull Request, a review process will be carried out by other developers. During this period, the contributor is expected to remain available to answer questions, address comments, and make any necessary changes based on the feedback received. Any changes within the PR should remain relevant to the original purpose of the change. If additional changes are required that are not directly related, it is better to open a separate Pull Request. Receiving feedback as part of the review is an integral part of the process, even when it is negative, and should be treated in a constructive and collaborative spirit. A merge will only be performed once the changes have been approved by the reviewers.
(not sure about even when it is negative
)
6b32224
to
90db56d
Compare
Fixed the unrelated commits. Fixed typing error. |
CONTRIBUTING.md
Outdated
@@ -0,0 +1,143 @@ | |||
# How to contribute as a developer | |||
|
|||
To contribute, fork and clone this repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expand on this somewhat:
"To contribute, please fork this repository to create your own copy that you can clone locally and push back to. You can use your fork to create pull requests for your code to be merged into this repository."
Something like that to better give an overview of the typical work flow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
CONTRIBUTING.md
Outdated
|
||
### Scope of code changes | ||
|
||
Code edits only touch the lines of code that serve the intended goal of the change. Big refactors should not be combined with logical changes, because these can become very difficult to review. If a change requires a refactor, create a commit for the refactor before (or after) creating a commit for the change. A Pull Request can contain multiple commits and can be merged with **Rebase and Merge** if these commits are meant to be preserved on the main branch. Otherwise, the regular method of merging is **Squash and Merge**. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be fair the document already states that PRs prepared to be merged as multiple commits can be so that only really leaves squashing for PRs that aren't prepared that way. PR merge style is more of a repository maintainence thing than a contributor issue really other than if they want to worry about preserving their commits or not.
Maybe reword the last sentence to "Otherwise, method of merging will be Squash and Merge.
Really we are only ruling out the github default of creating an additional merge commit and allowing none linear history.
CONTRIBUTING.md
Outdated
|
||
### Style of code changes | ||
|
||
Code edits should fit the nearby code in ways that the code style reads consistent, unless the original code style is bad. The original game code uses c++98, or a deviation thereof, and is simple to read. Prefer to not use new fancy language features where not absolutely necessary to get a particular change done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove the word "fancy" from describing language features as it implies a value judgement on their suitability and just leave it as something like "Prefer not to use newer language features unless required to implement the desired change."
Regarding the guidelines you both refer to, I read them as pretty much saying the same thing in different ways.
auto widget = MyWidgetFactory<WidgetType>(); // good
WidgetType widget = MyWidgetFactory<WidgetType>(); // acceptable
auto widget = MyWidgetFactory(); // bad
WidgetType widget = MyWidgetFactory(); // good
* [GITHUB] | ||
* [LINUX] | ||
|
||
If the Pull Request is meant to be merged with rebase, then a note for **Merge with Rebase** should be added to the top of the text body, to help identify the correct merge action when it is ready for merge. All commits of the Pull Request need to be properly named and need the number of the Pull Request added as a suffix in parentheses. Example: **(#333)**. All commits need to be able to compile on their own without dependencies in newer commits of the same Pull Request. Prefer to create changes for **Squash and Merge**, as this will simplify things. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm personally not a fan of adding the PR number to the commit messages or generally referring to github or this repo specific things in them as if cloned or forked they won't lead back to the relevant items at all and will end up meaningless. Referring to issues in PRs and PRs in issues is fine as they are (mostly) locked to the repo in question and won't be split from each other.
90db56d
to
7172e21
Compare
Great addition. I would change the second to last line into:
|
|
||
### Language style guide | ||
|
||
*Work in progress. Needs a maintainer. Can be built upon existing Code guidelines, such as the "Google C++ Style Guide".* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need a maintainer for this? Can't we just say we use Google C++ Style Guide, except for indentation. For indentation, we use the same as the existing one (until we clean that up). Anyone objects to this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should go by the c++ standard guidelines not Google's style guide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C++ core guidelines are made by the creators and maintainers of c++ and c++ compilers. They set the ideal for safe, readable, efficient and maintainable code.
Google's guidelines are set by Google for people maintaining their codebase that is already written with that style. They're riddled with designs that are generally known to be bad practice but make sense for their situation.
Also with the c++ standard guidelines we can use GSL https://github.com/Microsoft/GSL to enforce guidelines rather than having to nit pull requests ourselves
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C++ core guidelines are made by the creators of C++, yes. But it shows that the creators don't have much real world experience if you ask me. Seeing the product they made also doesn't exactly inspire confidence.
An example is rule F.18: For “will-move-from” parameters, pass by X&& and std::move the parameter
, which will make common code very unreadable for anyone not experienced in rvalues.
Google's guideslines have some quirks like the 80 char limit and indentation, which we can easily exclude. Other than that, what bad practices do you see there? If you mean exceptions, it's pretty common for games not to use exceptions.
But I understand that this is a very subjective thing, maybe a poll would make sense here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C++ core guidelines are made by the creators of C++, yes. But it shows that the creators don't have much real world experience if you ask me. Seeing the product they made also doesn't exactly inspire confidence.
An example is rule
F.18: For “will-move-from” parameters, pass by X&& and std::move the parameter
, which will make common code very unreadable for anyone not experienced in rvalues.Google's guideslines have some quirks like the 80 char limit and indentation, which we can easily exclude. Other than that, what bad practices do you see there? If you mean exceptions, it's pretty common for games not to use exceptions.
But I understand that this is a very subjective thing, maybe a poll would make sense here.
The move semantics are for efficiency and safety. Avoids unnecessary copying, constructing and deleting.
Don't have real world experience?! C++ is awesome, impressive and arguably the best tool for what it's designed for. Rust is probably the only contender but you can't expect a language that prioritizes backwards compatibility and not breaking abi thats existed for decades to be as clean as a new language. The fact that we're able to compile our two decade old code with modern compilers is a testament to the power of its portability.
https://www.reddit.com/r/cpp/comments/zajjsk/cpp_core_guidelines_or_googles_c_style_guide/
The expected comment format is | ||
|
||
``` | ||
// TheSuperHackers @keyword author DD/MM/YYYY A meaningful description for this change. | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this optional, at least for short comments, or for @info comments? In my opinion this is just distracting and doesn't supply information that isn't in the git history.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
First version of the CONTRIBUTE.md
Please let me know if something is unclear or missing, besides the things that are documented as being Work in Progress.