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

Jump to bottom

Add review guidelines to the default PR template #1177

Closed
wants to merge 7 commits into from
Closed

Add review guidelines to the default PR template #1177

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a209918
Add review guidelines to the default PR template
teor2345 Oct 19, 2020
a2208bd
Apply suggestions
oxarbitrage Oct 19, 2020
dfd1c07
Add a Follow Up Work section to the PR template
teor2345 Oct 27, 2020
91b6c40
Mention design RFCs in the PR template
teor2345 Oct 27, 2020
8128471
Put key PR review questions in bold
teor2345 Oct 27, 2020
1f79dbd
Tweak PR review "skip task" process
teor2345 Oct 27, 2020
518ae6b
Update .github/pull_request_template.md
yaahc Oct 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
46 changes: 43 additions & 3 deletions .github/pull_request_template.md
View file
Open in desktop
@@ -1,8 +1,7 @@
<!--
Thank you for your Pull Request. Please provide a description above and review
the requirements below.
Thank you for your Pull Request.

Bug fixes and new features should include tests.
Please provide a description above and fill in the information below.

Contributors guide: https://zebra.zfnd.org/CONTRIBUTING.html
-->
Expand All @@ -26,3 +25,44 @@ the code change.
<!--
Please link to any existing GitHub issues pertaining to this PR.
-->

## Review Guidelines (for Reviewer)
<!--
This is a flexible checklist for the reviewer to fill in.

Developers:
Add extra tasks to the review using list items.
Skip review tasks using ~~strikethrough~~.
If you want this pull request to have a specific reviewer, tag them in the list of reviewers.

Reviewer:
This checklist can help you do your review.
Add or skip tasks as needed.
-->

**Does this pull request improve Zebra?**
Copy link
Contributor

Choose a reason for hiding this comment

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

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?**

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
**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.
-->

Copy link
Contributor

@yaahc yaahc Oct 27, 2020 •
edited

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@teor2345 teor2345 Oct 28, 2020 •
edited

Choose a reason for hiding this comment

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

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?**

Copy link
Contributor

Choose a reason for hiding this comment

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

👍


- [ ] Pull Request
- The pull request is complete: code, documentation, and tests
teor2345 marked this conversation as resolved.
Show resolved Hide resolved
- The PR contains changes to related code (unrelated changes can be split into another PR)
Copy link
Contributor Author

@teor2345 teor2345 Oct 28, 2020 •
edited

Choose a reason for hiding this comment

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

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)

- Any follow-up tasks are listed in a GitHub issue
oxarbitrage marked this conversation as resolved.
Show resolved Hide resolved

- [ ] Code
- The code implements the specifications or design documents
teor2345 marked this conversation as resolved.
Show resolved Hide resolved
- Changes from the specifications or design documents are explained in comments
- The code is readable, and does not appear to have any bugs
yaahc marked this conversation as resolved.
Show resolved Hide resolved
- Any known issues or limitations are documented

- [ ] Documentation
- Docs summarise how the code should be used
teor2345 marked this conversation as resolved.
Show resolved Hide resolved
- Docs reference specifications or design documents
- New public code has doc comments: `#![deny(missing_docs)]`
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 want to add #![deny(missing_docs)] to every zebra crate (#453), then we can remove this checklist item.

- Complex private code has doc comments

- [ ] Tests
- The tests make sure the code implements the Zcash consensus rules
teor2345 marked this conversation as resolved.
Show resolved Hide resolved
- Consensus rules have success and failure tests
oxarbitrage marked this conversation as resolved.
Show resolved Hide resolved
- Edge cases and errors have tests
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we delete this checklist item?

Suggested change
- Edge cases and errors have tests

- The consensus rules are tested on the block lists in `zebra_test::vectors`
- Tests cover mainnet and testnet