Pull Requests
Procedure
It is preferred to open an issue before starting the work on a pull request. If the pull request closes an issue self assign to the ticket before starting to work on it to ensure no two contributors are accidentally working on the same thing.
After requested changes have been made you as the reviewee are expected to request a rereview from the reviewer. This can be done incremenatlly, however in that case the reviewer should be informed of what aspects have been addressed.
After approval the pull request will be merged by the reviewer.
Target Branch
All pull requests should be targeting the main branch. Changes to dev without an associated pull request towards main should be avoided.[1]
Any PR branch may be merged into the dev branch (nightly) at any time by a maintainer without a separate pull request or review, when this is done the 'nightly' tag should be applied.
Subsequent changes should be added to the pull request targeting main and merged or cherry picked into dev.
Once the pull request has been reviewed and merged into the main branch it will be automatically synced into the dev branch.[2]
Description
The description is for the communication between the reviewee and the reviewers.
It doesn't have to contain all of the following aspects however addressing them will speed up the review process and a reviewer may request you to provide them at their descretion.
Descriptive Title
Titles should contain the type and content of the change. Someone only reading the title should be able to understand what the pull request is about.
Positive Examples: "Fixed XYZ bug", "Updated CI to do Y", "Added X service"
Negative Exmaples: "Updated XYZ.cs", "changes"
Change Description
A description of the changes at a higher abstraction level than the code, including backwards compatibility. A reviewer should be able to understand the general shape of the changes from reading this.
Reason for the change
An explanation of why the changes are necessary and what benefit they bring. This point can also be addressed by linking to the issue that the pull request closes.
Steps to verify
Steps to observe the changed behavior and a description of the intended behavior for codepaths which do not have test coverage. This point can also be addressed by linking to the issue that the pull request closes.
Screenshots
Screenshots of UI changes if applicable.
Additional Info
Anything else that is relevant to the review or organizational process, e.g. request merging into dev, conditions for merging, or a specific merge procedure (merge vs squash merge etc.)
Nice to have
The entries in this section are entirely optional and not generally expected, however in certain cases they can be useful.
- Graphs visualizing e.g. the control flow
Code Content
Pull requests should follow the code of conduct as well as the developer guide.
Change scope
Each pull request should be limited to changes that fall within a well-defined scope. Whenever feasible, use multiple smaller PRs rather than one larger PR. Stacked pull requests may be used to group related changes.
Nonetheless, a PR that meets all the acceptance criteria (tests, improves code quality, etc.) should not be rejected solely on the basis of size or difficulty of review.
Functionality
Pull requests or pull request stacks should contain complete changes that address the entire issue they are solving and should not introduce any new issues. If the changes in a pull request uncover or exaggerate an exisitng issue it should be resolved as part of the pull request or pull request stack.
INFO
Exceptions can be made in coordination with the reviewer.
Tests
Changes should be written with testability in mind. Newly added business logic should include new unit tests.[3]
Pull requests should be passing all tests.
Backwards compatability
Breaking changes[4] must be disclosed and may delay the merging of the pull request.
Changelog
Any pull request that contains user[5] relevant changes[6] should include a changelog entry. Guidelines for adding to the changelog can be found here.
INFO
If you are a new contributor and are unsure how to correctly do so, feel free to ask, and you will be assisted.
The pull request towards main can be a draft ↩︎
When there are no merge conflicts ↩︎
When extending existing classes which do not have test coverage adding tests is not a must, but definitely a nice to have ↩︎
API changes in nuget packages which are not backwards compatible, or additional dependencies (in the case of the App or CLI dependencies that the user must install) ↩︎
For the App, CLI and Unpacker that is the end user, for ModKit, Common, Core and RED4 that is the developer using the nuget packages ↩︎
Relevant changes are directly observable by the consumer ↩︎