change(pr): Add an author checklist to the PR template

[teor2345]

Motivation

What are the most important goals of the ticket or PR?

We've had a few discussions over the last few months about the PR review process. I've tried to find the top 5 things that authors and reviewers need to check, based on our discussions. (And comments on PRs from reviewers and PR authors.)

This is part of ticket #7738.

PR Author Checklist

Check before marking the PR as ready for review:

• Will the PR name make sense to users?
  • Does it need extra CHANGELOG info?
  • Does the PR have a priority label?
• Have you added or updated tests?
• Is the documentation up to date?

This is an internal GitHub change, so it doesn't need most of these things.

Specifications

https://docs.github.com/en/communities/using-templates-to-encourage-useful-issues-and-pull-requests/creating-a-pull-request-template-for-your-repository

Complex Code or Requirements

Not really, but finding the top 5 things is tricky. Let's see how it goes and update it over time?

Solution

• Add a PR author checklist, and move some reviewer tasks into it
• Update the reviewer checklist
• Add a testing section
• Remove some redundant or unused comments from other sections

Testing

I manually checked the preview looks ok:
https://github.com/ZcashFoundation/zebra/blob/pr-checklist/.github/pull_request_template.md

This template will be fully tested when it is merged.

Review

This is a routine change that every Zebra developer should review.

Reviewer Checklist

Check before approving the PR:

• Does the PR scope match the ticket?
• How do you know it works? Do the tests cover the PR motivation?
• Are there any blockers to merging this PR?
  • Can some changes go in new tickets or PRs?

And check the PR Author checklist is complete.

Follow Up Work

Ticket #7738 lists some other ticket templates and docs, but I did this one first, because everything in here doesn't need to go in the ticket templates.

[upbqdn]

I find it hard to express the answers to some questions in their checkboxes. For example, the checkboxes of the following two questions

• Does the PR have a priority label?
• Does it need extra CHANGELOG info?

seem to have the opposite behavior because:

• The answer to the first q is "yes" for a correct PR, so I would intuitively check the box.
• However, the answer to the second q is "no" for a correct PR, and now I'm stuck thinking about whether I should check the box or not.
• Also, the answer to the second q is "yes" for an incorrect PR, and I'm stuck again thinking of the implications of checking the box.

One solution that comes to my mind is to phrase the questions so that the respondent can indicate the correctness of the PR by checking the box. A checked box means "yes, all good", and an unchecked box means "no, something bad", or vice versa.

Besides the two questions above, I also got stuck on these:

• Have you added or updated tests? (Some PRs don't need any.)
• How do you know it works?
• Are there any blockers to merging this PR?
• Can some changes go in new tickets or PRs?

[teor2345]

One solution that comes to my mind is to phrase the questions so that the respondent can indicate the correctness of the PR by checking the box. A checked box means "yes, all good", and an unchecked box means "no, something bad", or vice versa.

I rephrased all the checkboxes in commit 2dff2b3 (#7832) including the others you had concerns about.

[teor2345]

• Have you added or updated tests? (Some PRs don't need any.)

I added a note that irrelevant boxes should be checked. But really it doesn't matter because they are irrelevant, and both the author and reviewer will make sure they are irrelevant.

There's no way to phrase all the items so they are relevant or conditional, unless we want to make the phrasing really complex, or repeat relevance in each one.

[upbqdn]

All looks good. Thank you for the refactor. I'm not approving so that we don't merge the PR before others review it.

[teor2345]

@oxarbitrage @gustavovalverde @mpguerra just checking if you have any feedback on the new PR template before it merges?

We can change it later if you don't have time to review now.

[mpguerra]

LGTM

[teor2345]

We've had 2 reviews, so let's move on with this, and change it later if needed.