-
Notifications
You must be signed in to change notification settings - Fork 531
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.
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
Clean up 'Contributing to Rust - Pull Requests' #870
Conversation
|
||
[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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
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. |
There was a problem hiding this comment.
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 ;)
I think it's not discoverable enough still ... I think to mark that as closed we should split up the contributing page into multiple smaller ones, then I'd be happy enough. As it is there's a lot of text to wade through to find what you're looking for. |
This is definitely much better than before though! |
Changed it to not close #866. |
Co-authored-by: Joshua Nelson <joshua@yottadb.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is somewhat of a big change - maybe someone else wants to take a look? but LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! LGTM :)
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a pull request, CI builds the compiler and tests it against the | |
a PR, CI builds the compiler and tests it against the |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The inconsistency bothered me
Also helps with #866.