Infos for contributers on prs and reviews in separate pages - #849
Infos for contributers on prs and reviews in separate pages#849JBludau wants to merge 9 commits into
Conversation
…ome (hopefully) useful questions
Co-authored-by: Adrien Taberner <56835712+Adrien-Tab@users.noreply.github.com>
Co-authored-by: Adrien Taberner <56835712+Adrien-Tab@users.noreply.github.com>
Co-authored-by: Adrien Taberner <56835712+Adrien-Tab@users.noreply.github.com>
Co-authored-by: Adrien Taberner <56835712+Adrien-Tab@users.noreply.github.com>
crtrott
left a comment
crtrott
left a comment
There was a problem hiding this comment.
I feel there is a lot of redundancy between prs.rst and reviews.rst - which is natural since what someone should take into account for writing a good PR, is what the reviewer should look for when reviewing it. But if folks see value in splitting it fine with me.
I would add on the review side explicitly
- Are implementation details accidentally exposed?
- Is there unnecessary fencing
- Are there unnecessary allocations/deallocations
| Communication | ||
| ------------- | ||
|
|
||
| - Am I using a helpful, neutral tone? | ||
| - Am I giving enough info to the author? |
There was a problem hiding this comment.
I'm not sure if this really belongs here.
I think this is more of a human interaction aspect, not really a review-specific thing.
| Final Questions | ||
| --------------- | ||
|
|
||
| - Does it definitely improve the current state? |
There was a problem hiding this comment.
I think current state is ambiguous.
It's already conveyed at the beginning of this page:
The goal of a review is to help the code contributor to improve the code while also checking if it is the best approach to the described problem.
Could mirror it to be something like Does it resolve the described problem
| - Does it adhere to design principles like SOLID,DRY? | ||
| - Does the design and variable naming fit into the rest of Kokkos? | ||
| - Is the current design restricting future design choices? Does/should it allow extension? | ||
| - Is any implicit dpenendency introduced? |
There was a problem hiding this comment.
| - Is any implicit dpenendency introduced? | |
| - Is any implicit dependency introduced? |
There was a problem hiding this comment.
I'm not sure where, but I think there could be a line stating to check whether any proposed functionality in a PR already exists in Kokkos. We want to avoid adding redundant functionality.
| --------------------- | ||
|
|
||
| - Is the PR title clear enough about the scope of the changes? | ||
| - Is clear what problem the PR is trying to resolve? |
There was a problem hiding this comment.
| - Is clear what problem the PR is trying to resolve? | |
| - Is it clear what problem the PR is trying to resolve? |
| ----------------- | ||
|
|
||
| - 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.
I had trouble understanding this sentence, does "they" refer to the reviewer? If so it might be better to replace it with "you"
| - 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 request changes if you 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. |
5 participants
This pr adds a Checklist-like set of questions for reviewing PRs
This was discussed in the developer meeting 2026 at SNL:
We talked about our resources for our own developers and the review and merging process.
While doing that several attendees noted that we could add more description, especially to the process of reviewing.
This originated from my own notes on reviews, so please tell me where stuff is unclear, I might be blind to it