Add a review checklist and suggest reviews as a way to get started with the project #1463
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jyn514
commented
Sep 14, 2022
Comment on lines
+1
to
+49
| # rust-lang/rust review checklist | ||
|
|
||
| - Does it have tests? | ||
| - Are the tests clear? | ||
| - Is each test checking a single failure case? | ||
| - Have relevant edge cases been checked? | ||
| - Do you find the PR confusing? | ||
| - Is the code confusing? Ask the author to leave more comments in the code or take a simpler approach. | ||
| - Is the change itself confusing? Ask the author to add more information to the PR description or commit message. for example: | ||
| - why are we making this change? | ||
| - is there an issue where we can read more about why the change is necessary? | ||
| - is the code changing a lot in a way that's not intuitive? | ||
| - are there unnecessary changes that don't look related to the PR title? | ||
| - Does it introduce a lot of duplicate code? | ||
| - Ask the author if they can think of a way to avoid the duplication | ||
| - If it's strictly necessary for some reason, ask them to leave a comment justifying why | ||
| - If it's not necessary, ask them why they think the duplication is better than the alternative | ||
| - Is the API hard to use? | ||
| - Is it hard to figure out when or how to call it? Suggest adding more documentation or examples. | ||
| - Does it require excessive boilerplate to use? | ||
| - If so, what benefits are we gaining? | ||
| - Can we make the common path more ergonomic? | ||
| - Is it easy to use it wrong by accident? Suggest making the correct way required by the type system (e.g. newtypes, typestate, callbacks) | ||
| - Does it use complex tuple types with more than two items? | ||
| - These should be avoided in favor of named structs for clarity and maintainability | ||
| - Does it use `Option` and `Result` appropriately? | ||
| - Library code should never panic; return `Option` or `Result` instead | ||
| - Option should be used when it is expected that | ||
| - Error types should implement `std::error::Error` | ||
| - Are useful common traits implemented on structs? | ||
| - `Clone`, `Debug`, `IntoIterator` and `Display` are some of the most generally useful traits | ||
| - Are sensible conversions into related types defined using the `From` trait?? | ||
| - If this adds a new feature, has there been a discussion of whether we should add this feature or not? | ||
| - For language features, there should be an RFC | ||
| - For new compiler features (e.g. unstable flags), there should be an FCP (https://rustc-dev-guide.rust-lang.org/implementing_new_features.html#implementing-new-features) | ||
| - For library features, there should be an ACP (https://std-dev-guide.rust-lang.org/feature-lifecycle/summary.html) | ||
| - For internal-only functions and tools, use your best judgement; feel free to have a conversation with the PR author. | ||
| - If this exposes a new API does it have documentation? | ||
| - Does the documentation link to other related methods, types and concepts? | ||
| - Are doc tests present to explain trickier or more elaborate methods? | ||
| - For library PRs, does it add or update examples? | ||
| - If this PR is performance-critical (WHAT DOES THIS MEAN), have benchmarks been added or evaluated? | ||
| - For compiler or rustdoc PRs, use `@bors try @rust-timer queue` when in doubt; the perf team has worked hard on having the perf bot generate a good summary | ||
| - For library PRs, ask the author whether they've run benchmarks | ||
| - Are methods marked `#[must_use]` where appropriate? | ||
| - Does the code organization make sense? | ||
| - Are related methods and types grouped together within each file? | ||
| - Does this functionality feel like it could fit in better with other code? | ||
| - Is this functionality sufficiently distinct that it should be split out into its own module or fuile? |
There was a problem hiding this comment.
I worry this will feel overwhelming; maybe we should say "not all bullet points will be applicable to each pr"?
There was a problem hiding this comment.
I think the key here is to add more structure. For example, many of these are only applicable to changes that make public facing APIs, others are focus on structs or methods specifically.
There was a problem hiding this comment.
I worry this will feel overwhelming
Maybe there could be a few top-level check-mark items. Then the others become suggestions that may be applicable or not.
| @@ -0,0 +1,49 @@ | |||
| # rust-lang/rust review checklist | |||
|
|
|||
| - Does it have tests? | |||
There was a problem hiding this comment.
These bullet points should be checklists, so they can be copy-pasted into comments and checked off.
| ### Reviewing PRs | ||
|
|
||
| Another way to contribute to the project is by reviewing existing PRs. We have a [review checklist] | ||
| you can use to get started. We suggest reviewing the PRs of existing contributors. Remember that |
There was a problem hiding this comment.
I don't think that suggesting reviewing the PRs of existing contributors is essential here; funneling beginners to help each other is very useful to get the easy nits out of the way quickly.
| @@ -192,6 +192,14 @@ sometimes be long. PRs are never merged by hand. | |||
| [r+]: https://github.com/rust-lang/rust/pull/78133#issuecomment-712726339 | |||
| [bors]: https://bors.rust-lang.org/queue/rust | |||
|
|
|||
| ### Reviewing PRs | |||
|
|
|||
| Another way to contribute to the project is by reviewing existing PRs. We have a [review checklist] | |||
There was a problem hiding this comment.
Suggested change
| Another way to contribute to the project is by reviewing existing PRs. We have a [review checklist] | |
| Reviewing existing PRs helps the project merge new features and bug fixes faster, and everyone is welcome to help! We have a [review checklist] |
| ### Reviewing PRs | ||
|
|
||
| Another way to contribute to the project is by reviewing existing PRs. We have a [review checklist] | ||
| you can use to get started. We suggest reviewing the PRs of existing contributors. Remember that |
There was a problem hiding this comment.
Suggested change
| you can use to get started. We suggest reviewing the PRs of existing contributors. Remember that | |
| you can use to get started. We suggest reviewing the PRs of existing contributors. Don't worry if you're new; |
alice-i-cecile
suggested changes
Sep 14, 2022
tshepang
reviewed
Sep 14, 2022
| - If so, what benefits are we gaining? | ||
| - Can we make the common path more ergonomic? | ||
| - Is it easy to use it wrong by accident? Suggest making the correct way required by the type system (e.g. newtypes, typestate, callbacks) | ||
| - Does it use complex tuple types with more than two items? |
There was a problem hiding this comment.
if there is an api-guideline about this, I would add the link here
tshepang
reviewed
Sep 14, 2022
| - Error types should implement `std::error::Error` | ||
| - Are useful common traits implemented on structs? | ||
| - `Clone`, `Debug`, `IntoIterator` and `Display` are some of the most generally useful traits | ||
| - Are sensible conversions into related types defined using the `From` trait?? |
There was a problem hiding this comment.
Suggested change
| - Are sensible conversions into related types defined using the `From` trait?? | |
| - Are sensible conversions into related types defined using the `From` trait? |
tshepang
reviewed
Sep 14, 2022
| - For new compiler features (e.g. unstable flags), there should be an FCP (https://rustc-dev-guide.rust-lang.org/implementing_new_features.html#implementing-new-features) | ||
| - For library features, there should be an ACP (https://std-dev-guide.rust-lang.org/feature-lifecycle/summary.html) | ||
| - For internal-only functions and tools, use your best judgement; feel free to have a conversation with the PR author. | ||
| - If this exposes a new API does it have documentation? |
There was a problem hiding this comment.
Suggested change
| - If this exposes a new API does it have documentation? | |
| - If this exposes a new API, does it have documentation? |
tshepang
reviewed
Sep 14, 2022
| - Does the code organization make sense? | ||
| - Are related methods and types grouped together within each file? | ||
| - Does this functionality feel like it could fit in better with other code? | ||
| - Is this functionality sufficiently distinct that it should be split out into its own module or fuile? |
There was a problem hiding this comment.
Suggested change
| - Is this functionality sufficiently distinct that it should be split out into its own module or fuile? | |
| - Is this functionality sufficiently distinct that it should be split out into its own module or file? |
JohnTitor
added
the
waiting-on-author
|
Triage: @jyn514 could you address the above review comments? Thanks! |
|
@jyn514 can you address the comments? otherwise I guess is better to merge this as is and open issues about the comments. |
|
don't have time to work on this in the near future |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
r? @alice-i-cecile