Skip to content
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

Jump to bottom

Clean up 'Contributing to Rust - Pull Requests' #870

Merged
merged 3 commits into from
Sep 11, 2020
Merged

Clean up 'Contributing to Rust - Pull Requests' #870

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d8ee191
Clean up 'Contributing to Rust - Pull Requests'
LeSeulArtichaut Sep 10, 2020
d76085f
Apply suggestions from code review
LeSeulArtichaut Sep 10, 2020
89acb4a
Fix typo
LeSeulArtichaut Sep 11, 2020
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
91 changes: 55 additions & 36 deletions src/contributing.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,41 +67,15 @@ in the appropriate provided template.

## Pull Requests

Pull requests are the primary mechanism we use to change Rust. GitHub itself
has some [great documentation][about-pull-requests] on using the Pull Request feature.
We use the "fork and pull" model [described here][development-models], where
contributors push changes to their personal fork and create pull requests to
Pull requests (or PRs for short) are the primary mechanism we use to change Rust.
GitHub itself has some [great documentation][about-pull-requests] on using the
Pull Request feature. We use the "fork and pull" model [described here][development-models],
where contributors push changes to their personal fork and create pull requests to
bring those changes into the source repository.

[about-pull-requests]: https://help.github.com/articles/about-pull-requests/
[development-models]: https://help.github.com/articles/about-collaborative-development-models/

Please make pull requests against the `master` branch.

Rust follows a _no merge-commit policy_, meaning, when you encounter merge
conflicts you are expected to always rebase instead of merge. E.g. always use
rebase when bringing the latest changes from the master branch to your feature
branch. Also, please make sure that fixup commits are squashed into other
related commits with meaningful commit messages.

GitHub allows [closing issues using keywords][closing-keywords]. This feature
should be used to keep the issue tracker tidy. However, it is generally preferred
to put the "closes #123" text in the PR description rather than the issue commit;
particularly during rebasing, citing the issue number in the commit can "spam"
the issue in question.

[closing-keywords]: https://help.github.com/en/articles/closing-issues-using-keywords

Please make sure your pull request is in compliance with Rust's style
guidelines by running

$ ./x.py test tidy

Make this check before every pull request (and every new commit in a pull
request); you can add [git hooks](https://git-scm.com/book/en/v2/Customizing-Git-Git-Hooks)
before every push to make sure you never forget to make this check. The
CI will also run tidy and will fail if tidy fails.

All pull requests are reviewed by another person. We have a bot,
[@rust-highfive][rust-highfive], that will automatically assign a random person
to review your request.
Expand All @@ -116,32 +90,77 @@ make a documentation change, add
to the end of the pull request description, and [@rust-highfive][rust-highfive] will assign
[@steveklabnik][steveklabnik] instead of a random person. This is entirely optional.

In addition to being reviewed by a human, pull requests are automatically tested
thanks to continuous integration (CI). Basically, every time you open and update
a pull request, CI builds the compiler and tests it against the
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
a pull request, CI builds the compiler and tests it against the
a PR, CI builds the compiler and tests it against the

Copy link
Member

Choose a reason for hiding this comment

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

This avoids having to decide between pull request and Pull Request

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think Pull Request vs pull request is a problem however

Copy link
Member

Choose a reason for hiding this comment

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

The inconsistency bothered me

[compiler test suite][rctd], and also performs other tests such as checking that
your pull request is in compliance with Rust's style guidelines.

Running continuous integration tests allows PR authors to catch mistakes early
without going through a first review cycle, and also helps reviewers stay aware
of the status of a particular pull request.

Rust has plenty of CI capacity, and you should never have to worry about wasting
computational resources each time you push a change. It is also perfectly fine
(and even encouraged!) to use the CI to test your changes if it can help your
productivity. In particular, we don't recommend running the full `x.py test` suite locally,
since it takes a very long time to execute.

After someone has reviewed your pull request, they will leave an annotation
on the pull request with an `r+`. It will look something like this:

@bors r+

This tells [@bors], our lovable integration bot, that your pull request has
been approved. The PR then enters the [merge queue][merge-queue], where [@bors]
will run all the tests on every platform we support. If it all works out,
will run *all* the tests on *every* platform we support. If it all works out,
[@bors] will merge your code into `master` and close the pull request.

Depending on the scale of the change, you may see a slightly different form of `r+`:

@bors r+ rollup

The additional `rollup` tells [@bors] that this change is eligible for to be
"rolled up". Changes that are rolled up are tested and merged at the same time, to
The additional `rollup` tells [@bors] that this change should always be "rolled up".
Changes that are rolled up are tested and merged alongside other PRs, to
speed the process up. Typically only small changes that are expected not to conflict
with one another are rolled up.
with one another are marked as "always roll up".

[rust-highfive]: https://github.com/rust-highfive
[steveklabnik]: https://github.com/steveklabnik
[@bors]: https://github.com/bors
[merge-queue]: https://buildbot2.rust-lang.org/homu/queue/rust

Speaking of tests, Rust has a comprehensive test suite. More information about
it can be found [here][rctd].
### Opening a PR
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I think I'd prefer for this to be a separate page to avoid making this one too long. But there's already a lot of sections, so maybe splitting them all up should be a separate PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I could do that in a separate PR


You are now ready to file a pull request? Great! Here are a few points you
should be aware of.

All pull requests should be filed against the `master` branch, except in very
particular scenarios. Unless you know for sure that you should target another
branch, `master` will be the right choice (it's also the default).

Make sure your pull request is in compliance with Rust's style guidelines by running

$ ./x.py test tidy --bless

We recommend to make this check before every pull request (and every new commit
in a pull request); you can add [git hooks](https://git-scm.com/book/en/v2/Customizing-Git-Git-Hooks)
before every push to make sure you never forget to make this check. The
CI will also run tidy and will fail if tidy fails.

Rust follows a _no merge-commit policy_, meaning, when you encounter merge
conflicts you are expected to always rebase instead of merging. E.g. always use
rebase when bringing the latest changes from the master branch to your feature
branch. Also, please make sure that fixup commits are squashed into other
related commits with meaningful commit messages.
Comment on lines +151 to +155
Copy link
Member

Choose a reason for hiding this comment

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

Once someone writes up #850, this should link there. But no need to solve two issues at once ;)


GitHub allows [closing issues using keywords][closing-keywords]. This feature
should be used to keep the issue tracker tidy. However, it is generally preferred
to put the "closes #123" text in the PR description rather than the issue commit;
particularly during rebasing, citing the issue number in the commit can "spam"
the issue in question.

[closing-keywords]: https://help.github.com/en/articles/closing-issues-using-keywords

### External Dependencies (subtree)

Expand Down