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 all commits
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 adhering to the :ref:`PR standards <pull-request-review-standards>`.


.. _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
63 changes: 63 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,63 @@
.. _pull-request-review-standards:

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

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

Before opening a pull request
=============================

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 meet the repository standards.

The PR should be as small as possible. However, the code included in the PR should be complete. It should complete an entire feature, but this doesn't necessarily mean it completes an entire issue.
However, all code merged into the ``dev`` branch should meet the repository standards and work as expected.

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 final. Marking your PR as draft means you are asking someone for an initial review, or if you want to get comments on your
initial design. If you put up a draft PR, indicate whether or not you are looking for initial reviews in the summary.

Finally, if you are addressing an existing issue, make sure that issue is `linked <https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword>`_ 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.
If your PR addresses a `Level-5 requirement <https://github.com/orgs/IMAP-Science-Operations-Center/projects/2/views/2>`_, there **must** be a corresponding issue linked.

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 straightforward and candid
#. If you leave a review, continue to follow up on replies to your questions or comments and review the changes you requested
#. It is nice, but not required, to provide examples for suggestions (particularly for things like name changes)
#. If you require a change to be addressed, add a "request changes" comment. If you make one of these comments, it means you are blocking the code review from merging until that change is addressed.
#. If you make a "request changes" comment, you must create a follow up review where you change that to an approving review (or make another "request changes" review). Please do this in a timely matter so you do not block the PR for longer than necessary

As an author:

#. If you would like to request a specific review from someone, make sure they are marked as a reviewer or called out in a comment on the review (by typing ``@<username>``)
#. You can request a review from the entire team or from a specific instrument team using the team ``IMAP-Science-Operations-Center/imap-sdc``

As a team:

#. All parties need to be respectful during code reviews
#. Don't take comments personally - treat everyone as a fellow team member working to produce excellent code, not as adversaries to defeat before you can merge
#. Be honest - if you disagree with someones comment, start a discussion on why that is the case

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.

#. Make sure you have left the review open for a sufficent amount of time to allow people to review - usually 3-5 days is enough.
#. 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.
#. 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)
#. 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.
#. At least one approval should come from the SDC team. Comments from external people should be treated the same way SDC comments are.
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