Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding documenation for pull request review standards #82

Merged
merged 9 commits into from
Aug 30, 2023
Merged

Adding documenation for pull request review standards #82

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6900c55
Adding documenation for pull request review standards
maxinelasp Aug 28, 2023
4add3d6
addressing PR comments
maxinelasp Aug 28, 2023
dccad9e
Adding notes from meeting discussion
maxinelasp Aug 28, 2023
9b3a8d3
removing unneeded section, adding link
maxinelasp Aug 28, 2023
d05fa5b
Spelling fixes and updates from PR
maxinelasp Aug 28, 2023
b82fd6f
Update docs/source/development/style-guide/review-standards.rst
maxinelasp Aug 29, 2023
80bfe3e
Update docs/source/development/style-guide/review-standards.rst
maxinelasp Aug 29, 2023
d3c3ec3
Update docs/source/development/style-guide/checklist-for-pull-request…
maxinelasp Aug 29, 2023
4dd8478
Clarifying draft PR paragraph
maxinelasp Aug 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion docs/source/development/style-guide/checklist-for-pull-requests.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@ is only a guide; it should not be treated as a fully comprehensive, foolproof li
and parts of it are subjective.

If the contributor/reviewer can answer "yes" to all the following questions, then conceivably the proposed changes are
acceptable and the PR can be reviewed and merged.
acceptable and the PR can be reviewed.

Before merging, make sure you have addressed all the comments in the review per our :ref:`PR standards <pull-request-review-standards>`.
maxinelasp marked this conversation as resolved.
Show resolved Hide resolved


.. _Checklist-for-Contributors:
Expand Down Expand Up @@ -71,6 +73,7 @@ Pertaining to the pull request:
* Does the PR have proper labels?
* Is the PR no longer a work in progress?
* Do all the automated checks pass?
* Does this post indicate the issue or explain the issue it is solving?


.. _pertaining-to-the-code-review:
Expand Down
51 changes: 51 additions & 0 deletions docs/source/development/style-guide/review-standards.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
.. _pull-request-review-standards:

Pull Request and Review Standards
---------------------------------

Before any code is merged into our code base, it will need to be put up for pull request (PR) and reviewed. The pull request should be created following
greglucas marked this conversation as resolved.
Show resolved Hide resolved
the :ref:`checklist for pull requests <checklist-for-contributors-and-reviewers-of-pull-requests>`.

Before opening pull request
maxinelasp marked this conversation as resolved.
Show resolved Hide resolved
===========================
maxinelasp marked this conversation as resolved.
Show resolved Hide resolved

Before you create the pull request, you should go through the :ref:`checklist for pull requests <checklist-for-contributors-and-reviewers-of-pull-requests>` to ensure
the proposed changes are required and up to our standards.
maxinelasp marked this conversation as resolved.
Show resolved Hide resolved

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
`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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like draft PR means it's not in final state but would like someone to give initial review.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a clarification, does that better line up with what you're thinking?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. Thank you!


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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

maxinelasp marked this conversation as resolved.
Show resolved Hide resolved

During review
=============

As a reviewer, please follow these rules of thumb:

#. Comments should be clear in addressing why you want to see the change
#. Comments should be polite, but straightfoward and candid
maxinelasp marked this conversation as resolved.
Show resolved Hide resolved
#. If you leave a review, continue to follow up on replies to your questions or comments and review the changes you requested
#. It is polite, but not required, to provide examples for suggestions (particularly for things like name changes)

Before merging
==============

Before merging, a pull request needs one approving review. While the review is open, anyone can make comments or request changes on the PR.

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.**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#. Do not merge until you have addressed all the comments on your review, even if you have an approval from someone.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This brings up two thoughts.

  1. Are we approving too easily right now? Maybe we should save approvals for when we feel like it has addressed all of our comments?

  2. 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#. If someone asked for changes beyond a nitpick, do not merge until you have an approval or thumbs up from them. This does not mean you need to change your code if you don't agree with them, but you should explain why you will not be making the changes and make sure they are ok with merging anyway.
maxinelasp marked this conversation as resolved.
Show resolved Hide resolved
#. You should go through the :ref:`pull request checklist <pertaining-to-the-code-review>`
#. You should ensure ALL checks on the PR pass (tests are passing)
greglucas marked this conversation as resolved.
Show resolved Hide resolved
#. If you have a lot of commits, clean up the commits by running a `rebase <https://git-scm.com/book/en/v2/Git-Branching-Rebasing>`_ and combining commits.
maxinelasp marked this conversation as resolved.
Show resolved Hide resolved

After merging
maxinelasp marked this conversation as resolved.
Show resolved Hide resolved
=============

Once you have merged your code:

#. Close the issue (if it exists)
#. Remove your branch if it is no longer needed
3 changes: 2 additions & 1 deletion docs/source/development/style-guide/style-guide.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,4 +41,5 @@ to these conventions.
naming-conventions
tools-and-library-recommendations
versioning
checklist-for-pull-requests
checklist-for-pull-requests
review-standards