Add review guidelines to the default PR template #1177

teor2345 · 2020-10-19T02:06:09Z

Motivation

In the Zebra development process, we haven't made explicit decisions about:

what should go in a pull request
what to do about follow-up tasks
what it means for code, documentation, and tests to be complete
how we make sure the PR matches any design RFCs
what things a reviewer should check

So we've been operating off conventions and assumptions we've learned from previous work.
Usually that works. But sometimes our assumptions clash, and we experience conflicts or confusion.

Solution

This change adds reviewer guidelines, a follow-up section, and design RFC questions to the default GitHub pull request template.

In reviews, we want to focus on this overall goal, so it appears first, in bold:
Does this pull request improve Zebra?

The rest of the change is a list of useful things to check, split up into categories. Each category starts with a key question in bold. These checklists are based on my experience writing and reviewing PRs - please feel free to suggest changes.

I tried to keep each checklist to 3-7 items, so that they are readable.
(I have left the part of the checklist in this PR, so you can see how it is rendered.)

Related Issues

Document the tests we need for each consensus rule #208 - Add contributor guidelines on testing
RFC Template: Test Plan

Review Guidelines (for Reviewer)

Does this pull request improve Zebra?

Pull Request
- The pull request is complete
- The PR contains changes to related code (unrelated changes can be split into another PR)
- Any follow-up tasks are listed in a GitHub issue
Documentation
- Docs summarise how the template should be used
- Docs reference specifications or design documents

Zebra Team Approval

Since this is a process change, let's make sure the whole team has reviewed it:

.github/pull_request_template.md

Co-authored-by: teor <teor@riseup.net>

oxarbitrage

I am ok in general as the reviewer guidelines are part of a flexible checklist and they are suggestions we will try to follow, not rules to apply to every PR.

.github/pull_request_template.md

teor2345 · 2020-10-27T11:18:47Z

I added design RFC questions, and follow-up work sections, so I've reset everyone's approvals.

yaahc

looks good, just a couple of suggestions that are mostly tone oriented

yaahc · 2020-10-27T21:02:01Z

.github/pull_request_template.md

+Add or skip tasks as needed.
+-->
+
+**Does this pull request improve Zebra?**


I think this should be removed, it reads as confrontational to me. I would expect every PR to improve zebra, otherwise it wouldn't have been written, and focusing on this question in the review checklist could be intimidating for new contributors, as though doubts were being cast upon their ability to improve and contribute to the codebase.

Suggested change

**Does this pull request improve Zebra?**

The goals of a PR and review are not stated, so authors and different reviewers can have conflicting goals - we've had PRs where that's happened.

For example, here is a non-goal:
Have we done everything possible to improve the code? Is it perfect?

And here are some unclear sub-goals:
When is documentation required?
When are tests required?

I'll try to rephrase the goal to be clearer.

Suggested change

**Does this pull request improve Zebra?**

**Overall Goal: Merged pull requests should improve Zebra.**

I think this still has those same unintentional connotation issues. How do you feel about this version?

- [ ] Description - **Does this PR focus on and achieve a clearly defined goal.** - The PR description highlights a concrete problem. - The solution directly solves the stated problem, rather than circumventing it (as possible). - The implemented code is not intended to be thrown away at a later date.

I'd like to discuss the goals of a PR, because it looks like we have some different goals here.

Does this PR focus on and achieve a clearly defined goal.

This is a useful goal, but it overlaps with a later goal:
The PR contains changes to related code (unrelated changes can be split into another PR)

The PR description highlights a concrete problem.

I think we should add this goal to the pull request section:
https://github.com/ZcashFoundation/zebra/pull/1177/files#r513157688

The solution directly solves the stated problem, rather than circumventing it (as possible).

This is a good goal, but I don't know how to check for it.
If we can find a good way to word it, let's add it to the Code section:
https://github.com/ZcashFoundation/zebra/pull/1177/files#diff-b2496e80299b8c3150b1944450bd81c622e04e13d15c411d291db0927d75fd6bR60

I also think there are good engineering reasons for circumventing some problems - as long as that's documented. See these other items:

Any known issues or limitations are documented

Any follow-up tasks are listed in a GitHub issue

The implemented code is not intended to be thrown away at a later date.

I think this is a non-goal: it's ok to add code that we plan to throw away, as long as that's clearly documented using comments and issues. See these related items:

The PR does everything listed in the tickets that it closes, or the designs that it finishes

Any follow-up tasks are listed in a GitHub issue

Any known issues or limitations are documented

I am also trying to keep the template short.

So I'd like to limit the number of sections and checklist items. Let's also look for things to delete - we should delete anything that isn't a top priority. (We can move details to the Zebra book if we need to.)

Should we delete this checklist item?

The item is ambiguous - it means different things to different people. So a PR template isn't a good place to discuss the goals of a review.

Let's move this discussion to next week's Zebra sync?

Suggested change

**Does this pull request improve Zebra?**

.github/pull_request_template.md

teor2345 · 2020-10-28T03:26:16Z

.github/pull_request_template.md

+
+- [ ] Pull Request
+  - **Is the pull request complete: code, documentation, and tests?**
+  - The PR contains changes to related code (unrelated changes can be split into another PR)


Suggested change

- The PR contains changes to related code (unrelated changes can be split into another PR)

- The PR description identifies a specific problem and solution

- The PR contains changes to related code (split unrelated changes into another PR)

teor2345 · 2020-10-28T03:34:01Z

.github/pull_request_template.md

+  - Any known issues or limitations are documented
+
+- [ ] Documentation
+  - **Do the docs summarise how the code should be used?**


Should we delete this checklist item?

Suggested change

- **Do the docs summarise how the code should be used?**

- **Docs summarise what the code does, and how it should be used**

teor2345 · 2020-10-28T03:34:33Z

.github/pull_request_template.md

+- [ ] Tests
+  - **Do the tests make sure the code implements the Zcash consensus rules?**
+  - Consensus rules have success tests, failure tests, and property tests
+  - Edge cases and errors have tests


Should we delete this checklist item?

Suggested change

- Edge cases and errors have tests

teor2345 · 2020-10-28T03:36:00Z

.github/pull_request_template.md

+- [ ] Documentation
+  - **Do the docs summarise how the code should be used?**
+  - Docs reference specifications or design documents
+  - New public code has doc comments: `#![deny(missing_docs)]`


I want to add #![deny(missing_docs)] to every zebra crate (#453), then we can remove this checklist item.

Co-authored-by: teor <teor@riseup.net>

teor2345 · 2020-10-30T07:15:02Z

I think this version of the template is far too long.
Reviewer guidelines belong in the book, not a template.

See #1235 for a much shorter version.

Add review guidelines to the default PR template

a209918

teor2345 added the A-infrastructure Area: Infrastructure changes label Oct 19, 2020

teor2345 requested review from hdevalence, dconnolly, yaahc and oxarbitrage October 19, 2020 02:06

teor2345 added this to Review in progress in 🦓 via automation Oct 19, 2020

teor2345 self-assigned this Oct 19, 2020

teor2345 commented Oct 19, 2020

View reviewed changes

.github/pull_request_template.md Show resolved Hide resolved

teor2345 commented Oct 19, 2020

View reviewed changes

.github/pull_request_template.md Outdated Show resolved Hide resolved

Apply suggestions

a2208bd

Co-authored-by: teor <teor@riseup.net>

oxarbitrage previously approved these changes Oct 19, 2020

View reviewed changes

teor2345 commented Oct 20, 2020

View reviewed changes

.github/pull_request_template.md Outdated Show resolved Hide resolved

teor2345 commented Oct 20, 2020

View reviewed changes

.github/pull_request_template.md Outdated Show resolved Hide resolved

teor2345 commented Oct 20, 2020

View reviewed changes

.github/pull_request_template.md Outdated Show resolved Hide resolved

teor2345 commented Oct 20, 2020

View reviewed changes

.github/pull_request_template.md Outdated Show resolved Hide resolved

Add a Follow Up Work section to the PR template

dfd1c07

teor2345 dismissed oxarbitrage’s stale review via dfd1c07 October 27, 2020 11:11

teor2345 added 3 commits October 27, 2020 21:15

Mention design RFCs in the PR template

91b6c40

Put key PR review questions in bold

8128471

Tweak PR review "skip task" process

1f79dbd

teor2345 requested a review from oxarbitrage October 27, 2020 11:29

yaahc reviewed Oct 27, 2020

View reviewed changes

teor2345 commented Oct 28, 2020

View reviewed changes

Update .github/pull_request_template.md

518ae6b

Co-authored-by: teor <teor@riseup.net>

teor2345 mentioned this pull request Oct 30, 2020

Add Review and Follow Up to PR Template #1235

Merged

teor2345 closed this Oct 30, 2020

🦓 automation moved this from Review in progress to Done Oct 30, 2020

teor2345 deleted the pr-review-guidelines branch March 21, 2022 21:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add review guidelines to the default PR template #1177

Add review guidelines to the default PR template #1177

teor2345 commented Oct 19, 2020 •

edited

oxarbitrage left a comment

teor2345 commented Oct 27, 2020

yaahc left a comment

yaahc Oct 27, 2020

teor2345 Oct 27, 2020

teor2345 Oct 27, 2020

yaahc Oct 27, 2020 •

edited

teor2345 Oct 28, 2020 •

edited

teor2345 Oct 28, 2020

yaahc Oct 28, 2020

teor2345 Oct 28, 2020 •

edited

teor2345 Oct 28, 2020 •

edited

teor2345 Oct 28, 2020

teor2345 Oct 28, 2020

teor2345 commented Oct 30, 2020

-**Does this pull request improve Zebra?**
+**Overall Goal: Merged pull requests should improve Zebra.**
+<!--
+PRs should make incremental improvements to Zebra.
+Reviewers can help PR authors write better code.
+But PRs don't need to be perfect. It's ok to have follow-up tasks.
+-->

	- The PR contains changes to related code (unrelated changes can be split into another PR)
	- The PR description identifies a specific problem and solution
	- The PR contains changes to related code (split unrelated changes into another PR)

	- Do the docs summarise how the code should be used?
	- Docs summarise what the code does, and how it should be used

Add review guidelines to the default PR template #1177

Add review guidelines to the default PR template #1177

Conversation

teor2345 commented Oct 19, 2020 • edited

Motivation

Solution

Related Issues

Review Guidelines (for Reviewer)

Zebra Team Approval

oxarbitrage left a comment

Choose a reason for hiding this comment

teor2345 commented Oct 27, 2020

yaahc left a comment

Choose a reason for hiding this comment

yaahc Oct 27, 2020

Choose a reason for hiding this comment

teor2345 Oct 27, 2020

Choose a reason for hiding this comment

teor2345 Oct 27, 2020

Choose a reason for hiding this comment

yaahc Oct 27, 2020 • edited

Choose a reason for hiding this comment

teor2345 Oct 28, 2020 • edited

Choose a reason for hiding this comment

teor2345 Oct 28, 2020

Choose a reason for hiding this comment

yaahc Oct 28, 2020

Choose a reason for hiding this comment

teor2345 Oct 28, 2020 • edited

Choose a reason for hiding this comment

teor2345 Oct 28, 2020 • edited

Choose a reason for hiding this comment

teor2345 Oct 28, 2020

Choose a reason for hiding this comment

teor2345 Oct 28, 2020

Choose a reason for hiding this comment

teor2345 commented Oct 30, 2020

teor2345 commented Oct 19, 2020 •

edited

yaahc Oct 27, 2020 •

edited

teor2345 Oct 28, 2020 •

edited

teor2345 Oct 28, 2020 •

edited

teor2345 Oct 28, 2020 •

edited