Add documentation how to author commits, PR reviews and CI #2017
Conversation
This should clarify the process of what happens after a PR has been opened by a contributor a little.
|
@swift-ci Please test |
|
|
||
| Prefer to squash the commits of your PR (*pull request*) and avoid adding commits like “Address review comments”. This creates a clearer git history, which doesn’t need to record the history of how the PR evolved. | ||
|
|
||
| We prefer to not squash commits when merging a PR because, especially for larger PRs, it sometimes makes sense to split the PR into multiple self-contained chunks of changes. For example, a PR might do a refactoring first before adding a new feature or fixing a bug. This separation is useful for two reasons: |
There was a problem hiding this comment.
Arguably those refactorings could (and should) be a separate PR. Though we're pretty lax on that generally 😅.
I also think squashing at merge is fine as long as it's only a single change, which I would really expect it to be for any new contributor. So IMO this section isn't really needed. Or if we do want it, should be worded slightly differently to make the main point about not having "address review comments" type commits, unless they're okay with them being squashed before merging.
There was a problem hiding this comment.
The way I phrased this was to first say that “Address review comment” commits should be avoided because I don’t want to (and will forget) to check the git history to decide whether to squash and merge or just merge a contributor’s PR.
After that I just added rationale for this decision because I think I’ve had 3 or 4 discussions so fare with contributors noting that we could just squash and merge and if we knew about this feature. So the second paragraphs is basically: Yes, we know there’s squash and merge but we don’t want to use that.
|
|
||
| ## Review and CI Testing | ||
|
|
||
| After you opened your PR, a maintainer will review it and test your changes in CI (*Continuous Integration*) by adding a `@swift-ci Please test` comment on the pull request. Once your PR is approved and CI has passed, the maintainer will merge your pull request. |
There was a problem hiding this comment.
| After you opened your PR, a maintainer will review it and test your changes in CI (*Continuous Integration*) by adding a `@swift-ci Please test` comment on the pull request. Once your PR is approved and CI has passed, the maintainer will merge your pull request. | |
| After opening your PR, a maintainer will review it and test your changes in CI (*Continuous Integration*) by adding a `@swift-ci please test` comment on the pull request. Once your PR is approved and CI has passed, the maintainer will merge your pull request. |
There was a problem hiding this comment.
I always spell Please with a capital P 😉
This should clarify the process of what happens after a PR has been opened by a contributor a little.