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.

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

Add guidance on how to review a pull request #1872

Merged
merged 16 commits into from Nov 26, 2020
Merged

Add guidance on how to review a pull request #1872

merged 16 commits into from Nov 26, 2020

Conversation

bouweandela
Copy link
Member

@bouweandela bouweandela commented Oct 19, 2020

Add guidance on how to review a pull request. Comments and/or questions are very welcome!

@bouweandela
Copy link
Member Author

@ESMValGroup/esmvaltool-developmentteam It would be great if you could have a look at this for the discussion in the workshop tomorrow at 1:30 PM.

@Peter9192
Copy link
Contributor

Hey @bouweandela very nice addition. I really like the description you added to the documentation. My main input for the discussion would be that I find the PR template to be a bit intimidating. I wonder if we can somehow make it considerably more concise (without sacrificing the scrutiny).

@Peter9192
Copy link
Contributor

Ah and I wonder whether we should include a little bit more on the bot, like who can (and is supposed to) trigger it? Currently, if I understand the text correctly, the scientific reviewer is supposed to do this (or run the recipe by themselves), is that right?

@bouweandela
Copy link
Member Author

Maybe we could move the checklist from the template to the review documentation? Reviewers would then need to copy/paste the relevant bits from there? The downside of that is that the author of the pull request might not see it, so it might feel like the reviewer is coming up with a lot of 'new' requirements when they start the review.

@stefsmeets
Copy link
Contributor

stefsmeets commented Nov 24, 2020

Hi @bouweandela , nice work on the guidelines for the reviewing process!

I have few points of concern:

  • One section I'm missing that would be useful, is Who is responsible for initiating the reviewing process?. Not everyone may feel comfortable with this or have the overview. I'd recommend contacting someone from the core team to guide the process as a fallback.

  • It is not immediately clear if the 'Checklist for scientific review' / 'Checklist for technical review' are meant for the reviewers or the authors. I'd just leave it as it was before and make sections related to the type of content being added. Tasks for the technical / scientific review should not be part of the PR template, but part of the descriptive guidelines.

  • The PR template is super long and I'm afraid it will turn into noise after a while. I would recommend to have the checklist simply be a checklist with easy to check off tasks like:

    • Create an issue to discuss what you are going to do
    • Pull request has a descriptive title that can be used in the changelog
    • The code follows the style guide
    • Documentation is available
    • YAML files have been checked with yamllint
    • Specify additional dependencies needed for the diagnostic script (link to contributing guidelines)

Items that need a longer description should really be mentioned in the contributing.md / contributing guidelines and not explained in detail in the PR template.

@bouweandela bouweandela added this to ESMValTool - needs review in November 2020 Nov 24, 2020
@bouweandela
Copy link
Member Author

Who is responsible for initiating the reviewing process?

@stefsmeets That's why I added How do I request a review of my pull request? to the frequently asked questions, I think that answers that question.

@bouweandela
Copy link
Member Author

@axel-lauer How do we proceed with the two branches containing guidelines? Shall I merge the doc_guidelines_review into this branch/PR so we have a single document to work on?

@bouweandela
Copy link
Member Author

Ah and I wonder whether we should include a little bit more on the bot, like who can (and is supposed to) trigger it? Currently, if I understand the text correctly, the scientific reviewer is supposed to do this (or run the recipe by themselves), is that right?

@Peter9192 At the moment I think both reviewers need to run the recipe themselves, definitely the scientific reviewer, because how are you going to look at the plots if you don't have the results? Indeed it would be good to add more information on the bot, but maybe @nielsdrost can do that in a separate pull request once it is available for general use?

@axel-lauer
Copy link
Contributor

@axel-lauer How do we proceed with the two branches containing guidelines? Shall I merge the doc_guidelines_review into this branch/PR so we have a single document to work on?

I think it would be better to have everything in one branch. Would be great if you could do the merge.

@bettina-gier
Copy link
Contributor

Ah and I wonder whether we should include a little bit more on the bot, like who can (and is supposed to) trigger it? Currently, if I understand the text correctly, the scientific reviewer is supposed to do this (or run the recipe by themselves), is that right?

@Peter9192 At the moment I think both reviewers need to run the recipe themselves, definitely the scientific reviewer, because how are you going to look at the plots if you don't have the results? Indeed it would be good to add more information on the bot, but maybe @nielsdrost can do that in a separate pull request once it is available for general use?

If the bot runs the complete recipe, would it be possible to have the plots available from its run for the scientific reviewer? Depending on the size of the recipe having every reviewer run it separately from the bot might be resource intensive. Otherwise some people might not be able to review if the project they work on is nearing its computation resource limit.

@SarahAlidoost
Copy link
Contributor

@bouweandela thank you. Some suggestions on the review guidance and pull request template:

  • Under the title Technical review:
    ....
    The technical reviewer also keeps an eye on the design and checks that no major design changes are made without the approval from the technical lead development team.

It is not clear what the important features are in the design. Can we add a link to some part of the documentation here, if any.

We can add a note that the pull request checklist in the template should be complete.

We can add a note that in GitHub notification, there is a category "review requested".

  • Under the title What if the author and reviewer disagree?
    ....
    When reviewing a pull request, try to refrain from making changes to the pull request yourself, unless the author specifically agrees to those changes, as this could potentially be perceived as offensive.

It is better to move it to the top under the title "Technical review".


Regarding the pull request template, I agree with the previous comments about making it concise. Here some suggestions:

  • yamllint:
    [ ] Please use yamllint to check that your YAML files do not contain mistakes
    This can be moved to style guide .

  • documentation:
    New or updated recipe/diagnostic:
    [ ] Documentation for the recipe/diagnostic is available in the doc/sphinx/source/recipes folder and an entry has been added to index.rst

    New or updated data reformatting script:
    [ ] Add a new dataset to the table in the documentation

These two items can be covered by documentation at the top.

  • numbers and unit:
    New or updated data reformatting script:
    [ ] The numbers and units of the data look physically meaningful

This item should be checked for recipes (new or updated), too. Also, I suggest to re-write it as: The numbers and units of the output look physically meaningful.

Copy link
Member

@nielsdrost nielsdrost left a comment

Choose a reason for hiding this comment

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

Nice work Bouwe!

doc/sphinx/source/community/review.rst Outdated Show resolved Hide resolved
doc/sphinx/source/community/review.rst Outdated Show resolved Hide resolved
@nielsdrost
Copy link
Member

Ah and I wonder whether we should include a little bit more on the bot, like who can (and is supposed to) trigger it? Currently, if I understand the text correctly, the scientific reviewer is supposed to do this (or run the recipe by themselves), is that right?

@Peter9192 At the moment I think both reviewers need to run the recipe themselves, definitely the scientific reviewer, because how are you going to look at the plots if you don't have the results? Indeed it would be good to add more information on the bot, but maybe @nielsdrost can do that in a separate pull request once it is available for general use?

Yes, see #1882

@nielsdrost
Copy link
Member

Ah and I wonder whether we should include a little bit more on the bot, like who can (and is supposed to) trigger it? Currently, if I understand the text correctly, the scientific reviewer is supposed to do this (or run the recipe by themselves), is that right?

@Peter9192 At the moment I think both reviewers need to run the recipe themselves, definitely the scientific reviewer, because how are you going to look at the plots if you don't have the results? Indeed it would be good to add more information on the bot, but maybe @nielsdrost can do that in a separate pull request once it is available for general use?

If the bot runs the complete recipe, would it be possible to have the plots available from its run for the scientific reviewer? Depending on the size of the recipe having every reviewer run it separately from the bot might be resource intensive. Otherwise some people might not be able to review if the project they work on is nearing its computation resource limit.

Yes, that is the intention. The bot posts a link to the output in the PR.

@stefsmeets
Copy link
Contributor

stefsmeets commented Nov 24, 2020

Just an idea that I mentioned during the workshop:
We could configure a bot to trigger when Ready for review is pressed to comment with a list of follow-up steps:

  • List the follow-up steps to be taken by the PR author (how to pick a reviewer + link to documentation)
  • List of tasks / check boxes to be performed by the Technical reviewer
  • List of tasks / check boxes to be performed by the Scientific reviewer
  • Maybe show the command to run the recipe with ESMValBot?

Edit: Can be done via github actions, there is a ready_for_review trigger:
https://docs.github.com/en/free-pro-team@latest/actions/reference/events-that-trigger-workflows#pull_request

@bouweandela bouweandela moved this from ESMValTool - needs review to In Progress in November 2020 Nov 24, 2020
Co-authored-by: Niels Drost <n.drost@esciencecenter.nl>
@axel-lauer axel-lauer mentioned this pull request Nov 25, 2020
@axel-lauer
Copy link
Contributor

This looks good to me. @bouweandela @zklaus @nielsdrost @Peter9192 @bettina-gier does anyone of you have further comments? Since I think of the user's guide as a living document I would propose to get this merged soon. In my opinion, it is important to have something to start with that we can then improve as we go.

Copy link
Contributor

@axel-lauer axel-lauer left a comment

Choose a reason for hiding this comment

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

Looks good! I think this is an excellent starting point that can be improved over time (just as the rest of the documentation in the user's guide).

@Peter9192
Copy link
Contributor

does anyone of you have further comments? Since I think of the user's guide as a living document I would propose to get this merged soon. In my opinion, it is important to have something to start with that we can then improve as we go.

I agree we should merge this soon, but I was under the impression that @bouweandela was still going to implement some of the previous feedback points. Particularly shortening the PR template bullet points and adding links to the documentation, and reformatting the table near the bottom with the main text -- adding headers so that we can link there from the PR template.

@bouweandela
Copy link
Member Author

Yes, I was planning on doing that, but I haven't had the time yet.

@bettina-gier
Copy link
Contributor

Agreed on merging this to have a first guideline in place, and it looks great as a first draft.

When rewriting the points in the template, if you really want the reviewers checking the boxes, maybe change the wording to not be "you" but to be indirect wording like you used in the scientific bullet points. Also agree on keeping them more concise, e.g.
Please use yamllint to check that your YAML files do not contain mistakes -> YAML files pass 'yamllint' checks

@bouweandela
Copy link
Member Author

Ok, I'll merge this now and use all comments in a new pull request!

@bouweandela bouweandela merged commit 0a2cd65 into master Nov 26, 2020
November 2020 automation moved this from In Progress to Merged Nov 26, 2020
@bouweandela bouweandela deleted the doc-review branch November 26, 2020 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

7 participants