[Dev] Add checklist to PR template?

[pitrou]

Describe the enhancement requested

I have noticed that the mamba project has a checklist in their PR template:
https://github.com/mamba-org/mamba/blob/main/.github/PULL_REQUEST_TEMPLATE.md

Do we want to add something similar or is that not useful?

Component(s)

Developer Tools

[pitrou]

@zanmato1984 @lidavidm @raulcd @kou @AlenkaF @amoeba Thoughts?

[zanmato1984]

I think we have something similar already.

For instance our Rationale for this change and What changes are included in this PR? are pretty much their Description and for all these times they are working fine imo. The linked issue in our PR has richer and more structural representation of "type" (label or github built-in "type") than their textual representation. (that said, the connection between our issue and pr are somehow "implicit" but this is another topic.)

I like their checkbox-based checklist though. It is handier and richer than our approach of writing down "Yes"-es under each question.

[lidavidm]

conda-forge does this too.

[kou]

I don't have a strong opinion for it but I feel CI checks instead of checklist are better. If a contributor who reads PR template, both of text description and checklist will work. If a contributor who doesn't read PR template, both of them will not work. (I feel that most contributors don't read PR template...) So auto checks by CI will be better if we want to enforce something.

[pitrou]

So auto checks by CI will be better if we want to enforce something.

I agree, but items such as "I have performed a self-review of my code" cannot really be checked by CI.

I feel that most contributors don't read PR template

That's possible. If there is a checklist, they won't have checked the items then, so we can notice immediately.

[raulcd]

I think checklists are nice both as a submitter and as a reviewer and I think they are friendlier to new contributors.
It's easier to check a button than understanding what do I have to put on a free-form text.
I agree with @zanmato1984 comments above that some of the checks on these checklist are similarly covered today either by our PR description or the linked issue. For example the Are these changes tested? section on our current PR description cover a similar case than the testing part presented on the checklist.
Maybe we could still have part of the PR Description as is (rationale, changes included) but add a checklist section for Are these changes tested? and Are there any user-facing changes?

[kou]

The current our PR template has "Please remove this line and the above text before creating your pull request". If a contributor doesn't read our PR template, they don't remove it and we can notice that they didn't read our PR template immediately.

BTW, how about using auto comment like apache/arrow-site#714 (comment) instead of PR template? We use PR description in the message of merged commit. We don't need the checklist in the commit message.

[pitrou]

BTW, how about using auto comment like apache/arrow-site#714 (comment) instead of PR template?

I feel like auto comments are less visible than the PR template. For example the "Minor" auto comment is frequently ignored by PR submitters.

We don't need the checklist in the commit message.

That can also be handled in the merge script.

[amoeba]

What are the most common issues for which a checklist might improve things and for whom might they improve things? An example checklist could for this repo be helpful for discussion.

I think the checklist structure may have more utility for new contributors whereas experienced contributors benefit from the free-text structure. For example, "Are these changes tested?" might be just a yes/no for a new contributor but I often find myself and other maintainers use free text there to explain the testing approach in more detail.

I could see adding a few checklist items to the existing PR template as a bit of an experiment while leaving most of what we currently have as free-text.

[pitrou]

I could see adding a few checklist items to the existing PR template as a bit of an experiment while leaving most of what we currently have as free-text.

That's a good point. I could try to come up with something.

[AlenkaF]

All the comments in this discussion have a valid point.
I don't have a strong preference but would go with the current PR template, if needed to decide. It does take more effort to write things down but then all the info added helps others to understand the PR better.

So the last suggestion with few checklists as an experiment sound reasonable.

[pitrou]

Ok, here is a draft: #47916 . Feel free to comment there.