Add pr/review guidelines #456
Merged
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
fnrizzi
reviewed
Sep 7, 2023
fnrizzi
reviewed
Sep 7, 2023
fnrizzi
reviewed
Sep 7, 2023
fnrizzi
reviewed
Sep 7, 2023
fnrizzi
reviewed
Sep 7, 2023
fnrizzi
reviewed
Sep 7, 2023
fnrizzi
reviewed
Sep 7, 2023
nmm0
requested changes
Sep 12, 2023
| .. toctree:: | ||
| :maxdepth: 1 | ||
|
|
||
| PRs and Reviews <prs-and-reviews.rst> |
There was a problem hiding this comment.
Just a note, if you adopt the below heading change you don't need to rename the link here. It doesn't matter too much though.
ajpowelsnl
requested changes
Sep 12, 2023
|
|
||
| - For bug fix PRs: add test which would catch the issue without the fix | ||
| - Do newly added tests have the correct granularity? | ||
| - Do tests have a suitable runtime or are unnecessarily large? |
There was a problem hiding this comment.
- Remove words after "runtime"
| Reviewer Behavior | ||
| ----------------- | ||
|
|
||
| - provide timely feedback and respond to changes by the author of the pull request in a reasonable amount of time; it's best to give feedback to pull requests as quickly as possible. |
There was a problem hiding this comment.
- After establishing what PR stands for, use PR throughout (vs. "pull request")
| ----------------- | ||
|
|
||
| - provide timely feedback and respond to changes by the author of the pull request in a reasonable amount of time; it's best to give feedback to pull requests as quickly as possible. | ||
| - only request changes if they are ready to resolve the request upon changes by the author of the pull request; stalling pull requests for requested changes that have been addressed is a problem. |
There was a problem hiding this comment.
- Here, do you mean, "please make sure your PR review comments / change requests are not redundant"
| - only request changes if they are ready to resolve the request upon changes by the author of the pull request; stalling pull requests for requested changes that have been addressed is a problem. | ||
| - only review pull requests that have been marked as ready; we have a bunch of pull requests that explore the feasibility of ideas and just need the CI to run. Similarly, pull requests should only be marked as "ready for review" if the author is reasonably happy with the status. If the author mostly seeks feedback on general design and direction, this should be clearly communicated in the pull request description (either "draft" or "ready for review"). | ||
| - mirror communication with pull request author outside of pull requests (on slack, in person, video calls, etc.) in comments to the pull request. | ||
| - contact authors directly if more clarification is needed. |
There was a problem hiding this comment.
- Are lines 72 and 73 saying the same thing?
| - only review pull requests that have been marked as ready; we have a bunch of pull requests that explore the feasibility of ideas and just need the CI to run. Similarly, pull requests should only be marked as "ready for review" if the author is reasonably happy with the status. If the author mostly seeks feedback on general design and direction, this should be clearly communicated in the pull request description (either "draft" or "ready for review"). | ||
| - mirror communication with pull request author outside of pull requests (on slack, in person, video calls, etc.) in comments to the pull request. | ||
| - contact authors directly if more clarification is needed. | ||
| - not be afraid of reviewing pull requests even if they are (slightly) outside their comfort zone. |
There was a problem hiding this comment.
- Maybe also suggest that potential reviewers can partner with an experienced Kokkos team member to perform review to mature reviewing skills?
Co-authored-by: Francesco Rizzi <[email protected]> Co-authored-by: Nicolas Morales <[email protected]>
nmm0
approved these changes
Feb 12, 2024
ajpowelsnl
approved these changes
Apr 1, 2024
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.
No description provided.