Skip to content

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.

Sign up for GitHub

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

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

Add CONTRIBUTING document #548

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b850356
Add CONTRIBUTE document
xezon Mar 30, 2025
2557e1d
Rename CONTRIBUTE to CONTRIBUTING
xezon Apr 19, 2025
94fb3b7
Fix review comments, add more infos
xezon Apr 19, 2025
fcb81a6
Fix precedence
xezon Apr 20, 2025
7172e21
Address OmniBlade review comments
xezon May 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
143 changes: 143 additions & 0 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,143 @@
# How to contribute as a developer

To contribute, 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.

## Code guidelines

### 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, method of merging will be **Squash and Merge**.

### 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 not to use newer language features unless required to implement the desired change. Prefer to use newer language features when they are considerably more robust or make the code easier to understand or maintain.

### Language style guide

*Work in progress. Needs a maintainer. Can be built upon existing Code guidelines, such as the "Google C++ Style Guide".*
Copy link

@helmutbuhler helmutbuhler May 31, 2025 •
edited
Loading

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?

Copy link

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link

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

Copy link

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.

Copy link

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/


### Precedence of code changes

Changes to Zero Hour take precedence over Generals, if applicable. When the changed code is not shared by both titles, then the change needs to be created for Zero Hour first, and then recreated for Generals. The implementation of a change for both titles needs to be identical or as close as possible. Preferably the Generals replica of a change comes with the same Pull Request. The Generals replica can be created after the Zero Hour code review has finished.


## Change documentation

User facing changes need to be documented in code, Pull Requests and change logs. All documentation ideally is written in the present tense, and not the past.

Good:

> Fixes particle effect of USA Missile Defender

Bad:

> Fixed particle effect of USA Missile Defender

When a text refers to a faction unit, structure, upgrade or similar, then the unit should be worded without any abbrevations and should be prefixed with the faction name. Valid faction names are USA, China, GLA, Boss, Civilian. Subfaction names can be appended too, for example GLA Stealth.

Good:

> Fixes particle effect of USA Missile Defender

Bad:

> Fixes particle effect of MD


### Code documentation

User facing changes need to be accompanied by comment(s) where the change is made. Maintenance related changes, such as compilation fixes, typically do not need commenting, unless the next reader can benefit from a special explanation. The comment can be put at the begin of the changed file, class, function or block. It must be clear from the change description what has changed.

The expected comment format is

```
// TheSuperHackers @keyword author DD/MM/YYYY A meaningful description for this change.
```
Comment on lines +51 to +55
Copy link

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.


The `TheSuperHackers` word and `@keyword` are mandatory. `author` and date can be omitted when preferred.

| Keyword | Use-case |
|------------------|-------------------------------------------------------------|
| @bugfix | Fixes a bug |
| @fix | Fixes something, but is not a user facing bug |
| @compile | Addresses a compile warning or error |
| @feature | Adds something new |
| @performance | Improves performance |
| @refactor | Moves or rewrites code, but does not change the behaviour |
| @tweak | Changes values or settings |
| @info | Writes useful information for the next reader |
| @todo | Adds a note for something left to do if really necessary |

Block comment sample

```
// TheSuperHackers @bugfix JAJames 17/03/2025 Fix uninitialized memory access and add more Windows versions.
memset(&os_info,0,sizeof(os_info));
```

Optionally, the pull request number can be appended to the comment. This can only be done after the pull request has been created.

```
// TheSuperHackers @bugfix JAJames 17/03/2025 Fix uninitialized memory access and add more Windows versions. (#123)
```

### Pull request documentation

The title of a new Pull Request, and/or commit(s) within, begin with a **[GEN]** and/or **[ZH]** tag, depending on the game(s) it targets. If a change does not target a game, then a tag is not necessary. Furthermore, the title consists of a concise and descriptive sentence about the change and/or commit, beginning with an uppercase letter and ending without a dot. The title ideally begins with a word that describes the action that the change takes, for example `Fix *this*`, `Change *that*`, `Add *those*`, `Refactor *thing*`.

Good:
```
[GEN][ZH] Fix uninitialized memory access in Get_OS_Info
```

Bad:
```
Minimal changes for successful build.
```

Currently established commit title tags are

* [GEN]
* [ZH]
* [CORE]
* [CMAKE]
* [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.
Copy link

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

Copy link

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.

Copy link
Author

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.

Copy link

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.

Copy link

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.

Copy link
Author

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.

Copy link

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.

Copy link

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

Copy link
Author

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point is that you are front loading effort to save time later that probably isn't going to be spent that often if at all. Anyhow, we are defacto following that contribution guideline regardless so while I don't really see the utility of it and don't agree with polluting the actual commits with github specific info, I don't see any point in arguing about it further either.


The text body begins with links to related issue report(s) and/or Pull Request(s) if applicable.

To write a link use the following format:

```
* Fixes #222
* Closes #333
* Relates to #555
* Follow up for #666
```

Links are commonly used for

* closing a related issue report or task when this pull request is merged
* closing another pull request when this pull request is merged

Some keywords are interpreted by GitHub. Read about it [here](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue).

The text body continues with a description of the change in appropriate detail. This serves to educate reviewers and visitors to get a good understanding of the change without the need to study and understand the associated changed files. If the change is controversial or affects gameplay in a considerable way, then a rationale text needs to be appended. The rationale explains why the given change makes sense.


### Pull request merging rules

Please be mindful when merging changes. There are pitfalls in regards to the commit title consistency.

When attempting to **Squash and Merge** a Pull Request that contains a single commit, then GitHub will default generate a commit title from that single commit. Typically this is undesired, when the new commit title is meant to be kept in sync with the Pull Request title rather than the Pull Request commit title. The generated commit title may need to be adjusted before merging the Pull Request.

When attempting to **Squash and Merge** a Pull Request that contains multiple commits, the GitHub will default generate a commit title from the Pull Request title. Additionally it will generate a commit description from the multiple commits that are part of the Pull Request. The generated commit description generally needs to be cleared before merging the Pull Request to keep the commit title clean.

When attempting to **Rebase and Merge** a Pull Request, then all commits will transfer with the same names to the main branch. Verify that all commit titles are properly crafted, with tags where applicable, trailing Pull Request numbers in parentheses and no unnecessary commit descriptions (texts below the commit title).


### Change log documentation

*Work in progress.*