Skip to content

Add documentation how to author commits, PR reviews and CI #2017

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

Merged
merged 1 commit into from
Aug 8, 2023
Merged

Add documentation how to author commits, PR reviews and CI #2017

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 15 additions & 1 deletion CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,20 @@ Generated source code is not formatted to make it easier to spot changes when re

If you want to modify the generated files, open the [CodeGeneration](CodeGeneration) package and run the `generate-swift-syntax` executable.

## Authoring commits

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:
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

- During review, the commits can be reviewed individually, making each review chunk smaller
- In case this PR introduced a bug that is identified later, it is possible to check if it resulted from the refactoring or the actual change, thereby making it easier find the lines that introduce the issue.

## 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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.

Copy link
Member Author

Choose a reason for hiding this comment

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

I always spell Please with a capital P 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

shudder


Only contributors with [commit access](https://www.swift.org/contributing/#commit-access) are able to approve pull requests and trigger CI.

## Additional Verification

swift-syntax has additional verification methods (see the sections below) that provide more extensive validation. They have a significant runtime impact on swift-syntax and are thus not enabled by default when building swift-syntax, but are enabled in CI. If CI fails and you are unable to reproduce the failure locally, make sure that `SKIP_LONG_TESTS` is not set and try enabling these validations.
Expand All @@ -51,4 +65,4 @@ When testing finds one of these failures, it will show you the syntax tree that

## Swift Version

We require that swift-syntax builds with the latest released compiler and the previous major version (e.g. with Swift 5.8 and Swift 5.7).
We require that swift-syntax builds with the latest released compiler and the previous major version (e.g. with Swift 5.8 and Swift 5.7).