Add CONTRIBUTING document

[xezon]

• Closes Create CONTRIBUTION document #350

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.

[zzambers]

suggestion (for CONTRIBUTE document, possibly different wording):
Unless there is a good reason for that, bugs (or improvements) which affect both [ZH] an [GEN], should be fixed in [ZH] first, backport then can be done to [GEN], but it should be merged only after corresponding fix is merged into [ZH]. (Unless change is done to both in single PR.)

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.)

[ViTeXFTW]

Should I write a draft for the PR and Changelog sections?
This way, we can align on what we want the final format (PR and changelog files) to be and potentially start to handwrite the missing changelog files since we started the project.

[xezon]

Should I write a draft for the PR and Changelog sections? This way, we can align on what we want the final format (PR and changelog files) to be and potentially start to handwrite the missing changelog files since we started the project.

We can keep discussing in your change log PR: #523

I am currently unable to keep up with reviews because of the volume.

[xezon]

All comments addressed.

[feliwir]

Contains unrelated commits

[feliwir]

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

[xezon]

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.

[OmniBlade]

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.

[xezon]

Maybe reword the last sentence to "Otherwise, method of merging will be Squash and Merge."

Fixed

[feliwir]

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?

[xezon]

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

[feliwir]

When did we decide on that?

[feliwir]

Core Guidelines say: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-auto
ES.11: Use auto to avoid redundant repetition of type names

[OmniBlade]

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

[xezon]

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."

[feliwir]

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

[OmniBlade]

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.

[xezon]

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.

[DevGeniusCode]

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)

[xezon]

Fixed the unrelated commits. Fixed typing error.

[OmniBlade]

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?

[xezon]

Fixed.

[OmniBlade]

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.

[OmniBlade]

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

[OmniBlade]

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.