Adding documenation for pull request review standards #82
Conversation
maxinelasp
requested review from
bourque,
sdhoyt,
greglucas,
tech3371,
bryan-harter,
laspsandoval and
GFMoraga
| `wip <https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-pull-requests#draft-pull-requests>`_. | ||
| Anyone looking at the PR will be able to quickly see it is not yet ready for review. | ||
|
|
||
| Finally, if you are addressing an existing issue, make sure that issue is linked in your PR. If there is not an existing issue, then you should either create an issue or address WHY you are opening the PR specifically. |
There was a problem hiding this comment.
We can all discuss this, but I think if the PR is an implementation for a level 5 requirement an issue needs to be created and linked to the PR. So there's some cases where just opening a PR without an issue and explaining in the PR is fine, but I think a level 5 implementation or redesign of any kind should be a special case where an issue is required so we can easily keep track of the history of each level 5 by their linked sub-tasks.
There was a problem hiding this comment.
Ok, I do like that. I will add a note about specific requirements. I'd also be ok with requiring an issue period unless it's a documentation/ github workflow/pre commit PR, but I didn't want to make that change without discussing it with the team
|
|
||
| Although only one approval is required, you must follow these rules: | ||
|
|
||
| #. If there is someone with a particular expertise or vested interest in your changes, **do not merge or close the pull request until they get a chance to review.** |
There was a problem hiding this comment.
Should we set a rule of thumb for how long you should keep a PR open to give people time to review? Like 5 working days unless it's time sensitive or you get confirmation that no one else is planning to review?
There was a problem hiding this comment.
I do like that, although I think 5 is a little long. Maybe 3? It would be good to add a note that if you don't get any reviews after 3-4 days, to ping a reminder in the PR or in the slack channel, or ask the person who is closest to the issue/best reviewer.
There was a problem hiding this comment.
Yeah 3 would be good. I just want to avoid a situation where someone gets a review and approval on the same day they open the PR, then merge it right away before others have a chance to look, so 3 days sounds good too.
|
Looks great! The only confusion I had was "marking the pull request as wip", The link works, but it leads me to draft PR. Which I think it is great, but is a 'wip' the same as 'draft'? |
|
Great job on this! I think this will be really helpful |
Yes it is, thank you for calling that out. I will fix that confusion. |
greglucas
added
the
Repo: Documentation
| Although only one approval is required, you must follow these rules: | ||
|
|
||
| #. If there is someone with a particular expertise or vested interest in your changes, **do not merge or close the pull request until they get a chance to review.** | ||
| #. Do not merge until you have addressed all the comments on your review, even if you have an approval from someone. |
There was a problem hiding this comment.
This brings up two thoughts.
-
Are we approving too easily right now? Maybe we should save approvals for when we feel like it has addressed all of our comments?
-
Should we change from self-merging to reviewer-merging. i.e. we don't merge our own code, only a reviewer that thinks it is satisfactory would do that?
There was a problem hiding this comment.
I think leaving it as self-merging is fine. A reviewer might not know that, for example, I really want a review from someone specific but haven't gotten it yet.
What I was trying to express with this comment is that if Matthew gives me an approval, but Greg has outstanding comments, I should not merge until I have addressed Greg's comments.
I usually give an approval as a "when you fix this" type thing. If it's a minor change that I trust the author to make without needing another review, than I will give an approval, but I still want them to make the changes I suggested. In my opinion, even if the PR has approving reviews, the comments should still be addressed (where addressed means either fixed or replied to with why they will not be fixed).
Do you or others have other opinions? I would worry about dragging out the review process for minor things - I think people won't approve something which they have larger concerns around. But if it's something minor, like suggesting additional documentation, then the reviewer doesn't necessarily need to go back and review a change after it happens. (e.g. if I say "please add a docstring here" then my approval stands for whatever docstring the author adds)
There was a problem hiding this comment.
I usually give an approval as a "when you fix this" type thing. If it's a minor change that I trust the author to make without needing another review, than I will give an approval, but I still want them to make the changes I suggested. In my opinion, even if the PR has approving reviews, the comments should still be addressed (where addressed means either fixed or replied to with why they will not be fixed).
I usually do the same, so I agree with this. This was mostly just a thought of "if we care enough about these things", but it sounds like possibly not, so lets let it organically evolve and only update this if we run into issues in the future.
Co-authored-by: Matthew Bourque <Matthew.Bourque@lasp.colorado.edu>, Greg Lucas <greg.m.lucas@gmail.com>
maxinelasp
force-pushed
the
PR-style-guide
branch
from
e20b714 to
d05fa5b
Compare
Co-authored-by: Greg Lucas <greg.m.lucas@gmail.com>
docs/source/development/style-guide/checklist-for-pull-requests.rst
Outdated
Show resolved
Hide resolved
Co-authored-by: Greg Lucas <greg.m.lucas@gmail.com>
…s.rst Co-authored-by: Greg Lucas <greg.m.lucas@gmail.com>
|
|
||
| If you want to work on the pull request or are not yet finished with the code, please indicate this by marking the pull request as a | ||
| `draft or WIP <https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-pull-requests#draft-pull-requests>`_ PR. | ||
| Anyone looking at the PR will be able to quickly see it is not yet ready for review. |
There was a problem hiding this comment.
I feel like draft PR means it's not in final state but would like someone to give initial review.
There was a problem hiding this comment.
I added a clarification, does that better line up with what you're thinking?
…PR-style-guide Adding documenation for pull request review standards
Change Summary
Adding a document to cover review standards
New Files