3di | Making and reviewing a pull request#6892
Conversation
kscottz
left a comment
kscottz
left a comment
There was a problem hiding this comment.
This is better than we have so I'll approve it but I made a couple of minor suggestions.
|
|
||
| * When you begin `reviewing a pull request <https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/about-pull-request-reviews>`__, leave a comment to let others know it is under review. | ||
|
|
||
| 2 Reviewing the pull request |
There was a problem hiding this comment.
The problem with PR reviews isn't necessarily that people don't know how to do them, it is they don't know how to do them in a kind, respectful, and productive manner. I had a rough ROSCon talk outline that I put together and I was going to advocate that we move to a more concrete model for PR review comments.
There are roughly six types of comments that belong in a PR: “Nit:”, "Issue:", "Suggestion:", "Question:", "Chore:","Praise/Thanks."
Reviewers should strive to provide clarity around:
- Suggestions (i.e. things that might help but are not required for approval)
- High level concerns (i.e. refactorings). It helps to frame these as questions. (Questions and Issues)
- Low level concerns (line level changes ⇒ use Nit, Suggestion, or Chore)
- Reviewers should be clear on changes that block / do not block on merging.
@christophebedard I'd be interested to hear your opinion here. Do you think we should be giving more guidance in those domain?
There was a problem hiding this comment.
100% agree; this section should cover this.
I would also mention trying to start with bigger concerns (e.g., wrong design) before starting to comment on minor concerns (e.g., variable naming and other nits). There's no point in nitpicking stuff if everything is going to change anyway because of some bigger design concern. This helps minimize the back and forth between the author and reviewer, which can be annoying and add to the overall time it takes for a PR to be reviewed.
There was a problem hiding this comment.
Thanks, folks -- this sounds very worthwhile.
@kscottz It sounds like you are suggesting a formal system of a sort with the 6 types of comments, especially if your points on high level/low level concerns actually reference the comment types.
If you could provide or direct me to some input on that, I can make an attempt at adding some new stuff under section 2 along the lines discussed here.
There was a problem hiding this comment.
I wouldn't quite think of these as a full fledged formal system, but rather a way to frame reviewing a pull request. They help clarify the expected outcome on the part of the reviewer. As you can probably tell we're not great at using them ourselves.
At a high level, we want reviewers to be clear on issues that block and do not block getting a PR reviewed.
- Nit (Nitpick) - tiny, non-blocking piece of feedback about style, naming, or readability and does block a PR from being merged, and the author can safely address or ignore it.
- Issue: highlights a specific problem like a bug or logical error, that should be paired with a suggested fix (possibly using the suggestion feature. Issues generally block getting a PR merged.
- Suggestion: an improvement to the current code structure, logic, or performance that aren't a style preference. These are nice to have but should not block getting a PR merged.
- Question: Asking for clarification to understand why a specific approach was taken.
- Chore: Refers to work that doesn't add new features or fix bugs, but is required to keep the repository healthy. Roughly this is like asking someone to change your oil while they are also replacing your car's timing belt. It is stuff that needs to get done and it might as well get done when the hood of the car is already open.
- Praise: Self explanatory, but it helps offset the other stuff.
There was a problem hiding this comment.
Great, thanks for that. Will see what I can do and turn round another draft for review.
…ng-a-PR.rst Co-authored-by: Katherine Scott <katherineAScott@gmail.com> Signed-off-by: Keith Kirkwood <keith.kirkwood@3di-info.com>
…ewing-a-PR.rst Co-authored-by: Christophe Bedard <bedard.christophe@gmail.com> Signed-off-by: Keith Kirkwood <keith.kirkwood@3di-info.com>
…ewing-a-PR.rst Co-authored-by: Christophe Bedard <bedard.christophe@gmail.com> Signed-off-by: Keith Kirkwood <keith.kirkwood@3di-info.com>
…ood-3di/ros2_documentation into making-reviewing-a-pr
|
@kscottz (cc @tfoote) There are 2 types of issues which keep popping up in our docs builds for the 3di staff:
Could you advise on these? |
3 participants
Description
Create new how-to articles for Making a pull request and Reviewing a pull request under new section Contributing to code.
These draw from several existing articles, refactoring the content and removing from the original location.
There will be an action at the end of the tickets under this issue to clean up, so that we don’t end up with any duplicates or dead ends.
Related to: #6828
Did you use Generative AI?
No
Additional Information
Documentation updates from 3di Information Solutions, agreed with Geoff and Tully.