feat: Update PR template to something people might actually use #243
Conversation
I notice that people largely ignore PR templates, *especially* big ones. I think that shrinking this down to just the essentials will increase the likelihood that people use it. In particular, I want to remove things like "All reviewers approved" and "CI build is green" that are generally already enforced by repo policies, "Delete working branch" (there's a setting for that if we want it!), and release instructions (should be in a central document linked from the README).
| @@ -1,14 +1,7 @@ | |||
| **Description:** | |||
There was a problem hiding this comment.
Is the thought here that everyone know to add a useful description?
There was a problem hiding this comment.
For a single-commit PR, GitHub automatically suggests the commit's body here. I think if people aren't putting in useful commit bodies, they're not going to make useful PR descriptions anyhow. (I don't think I've ever seen someone use this prompt, in practice!)
| @@ -1,39 +1,8 @@ | |||
| **Description:** Describe in a couple of sentences what this PR adds | |||
There was a problem hiding this comment.
I'm not sure if there is a relevant OEP, or if we should just create an ADR in this repo, but the cookiecutter is making default decisions across repos, so it would be worth vetting changes like this across a wider audience.
There was a problem hiding this comment.
I've made an identical PR for event-bus-kafka so we can try it out there: openedx/event-bus-kafka#29
There was a problem hiding this comment.
I don't think an OEP or ADR is necessary because I think this change and its commit message are self-documenting. That said, it would be fair to circulate the PR around a few public Slack channels in order to expose it to a wider audience.
|
Bringing over an idea from the README template (#244): what about "poison pills" in the template? Content that needs to be removed, and can be detected easily by checks? |
|
I'm not fond of the idea of creating more work for people who are already doing the right thing. Maybe if it was really easy to delete the marker? Or a checkbox that shouldn't be checked, heh. :-) |
|
|
||
| **Merge checklist:** | ||
| - [ ] All reviewers approved | ||
| - [ ] CI build is green | ||
| Check off if complete or not applicable: | ||
| - [ ] Version bumped | ||
| - [ ] Changelog record added | ||
| - [ ] Documentation updated (not only docstrings) | ||
| - [ ] Commits are squashed |
There was a problem hiding this comment.
| - [ ] Commits are squashed | |
| - [ ] Commits are squashed into one or more [conventional commits](https://open-edx-proposals.readthedocs.io/en/latest/best-practices/oep-0051-bp-conventional-commits.html) |
There was a problem hiding this comment.
Oh, Phil raised an interesting question in chat -- we already have a commitlint CI check. Still worth the extra text? Could go either way.
There was a problem hiding this comment.
Great point Phil. I vote: destroy the extra text 🔥🔥🔥🔥🔥
There was a problem hiding this comment.
The one nice thing about including the link to Conventional Commits is for newbies. I find myself having to find this link repeatedly to explain conventional commits to people who are failing this check. Not necessarily advocating one way or another just putting in the thought.
|
BTW: the merge conflicts in CHANGELOG.rst are the primary reason for tools like towncrier and https://github.com/nedbat/scriv. |
|
Less is more. |
Co-authored-by: Kyle McCormick <[email protected]>
| solutions, hacks, quick-and-dirty implementations, concerns about | ||
| migrations, etc. | ||
| - [ ] Commits are squashed into one or more [conventional commits](https://open-edx-proposals.readthedocs.io/en/latest/best-practices/oep-0051-bp-conventional-commits.html) | ||
| - [ ] Noted any: Concerns, dependencies, migration issues, deadlines, tickets, testing instructions |
There was a problem hiding this comment.
| - [ ] Noted any: Concerns, dependencies, migration issues, deadlines, tickets, testing instructions | |
| - [ ] Noted any: Concerns, dependencies, migration issues, deadlines, tickets | |
| - Either: | |
| - [ ] Manually tested, with steps and/or screenshots provided | |
| - [ ] Manual testing is not necessary |
At risk of bloating the list: Did you test this, and if so, how? is probably the number-one question I ask in PR reviews. I love that OpenCraft's PR template includes it as an item and I think it makes reviewing their PRs much easier. I suggest adapting it here.
There was a problem hiding this comment.
Oh yeah, good point. Testing deserves a top-level bullet point. What about this instead, somewhere higher in the list?
- [ ] Automated tests added/updated, or manual testing instructions provided
There was a problem hiding this comment.
I think those are not mutually exclusive. If a reviewer adds a user-facing feature, I'd want them to both add unit tests and check that it works manually.
Then, there are some changes where automatic testing isn't possible (eg CSS changes) and some changes where testing just isn't required (eg reformatting a file).
| @@ -1,39 +1,8 @@ | |||
| **Description:** Describe in a couple of sentences what this PR adds | |||
There was a problem hiding this comment.
As an alternative to this PR: Why don't we delete this template from the cookiecutter, and instead add the PR template as an org-wide default to the .github repository? That way, updates to it will immediately apply to all repos without an explicit PR template, rather than only new repos.
There was a problem hiding this comment.
...I had no idea you could have an org-wide PR template! I actually really like that, because 1) I am generally suspicious of the maintenance burden that results from each new file the cookiecutter produces, and 2) it still allows customization if a repo has special requirements. I may open a new PR for .github.
There was a problem hiding this comment.
Nice. The only wrinkle is that not every repo has a changelog or versioning. But I'm sure you could figure out some creative wording to gloss over that.
There was a problem hiding this comment.
Speaking of that, how do you want folks to deal with check boxes that just don't apply to their PR?
Remove them? Strikethrough? Check them anyway?
There was a problem hiding this comment.
Maybe the org-wide one should end with "(If any of these are never applicable to this repo, please make a copy of this org-wide pull request template and trim it as appropriate.)" 😆 It's pretty hard to have something both comprehensive and relevant.
For inapplicable ones I personally tend to just check them off in the spirit of keeping things low-effort. I've suggested the language "Check off if complete or not applicable:" which hopefully would disambiguate that in practice.
There was a problem hiding this comment.
(If any of these are never applicable to this repo, please make a copy of this org-wide pull request template and trim it as appropriate.)
I like the sentiment but I fear that this would just take up space in the template and be ignored.
"Check off if complete or not applicable:" which hopefully would disambiguate that in practice.
How about "Delete if not applicable?"
I think "I did this" versus "I don't think it's applicable" is a useful thing to communicate to reviewers. Particularly around documentation and manual testing.
Unless, of course, the purpose if this template is not to tell reviewers things, but is to remind authors to do things, in which case just checking the non-applicable boxes works fine.
There was a problem hiding this comment.
Yeah, more text is probably not the answer. :-P
I mostly think about it as reminding authors, although it's great for reviewers too. So I'd prefer ticking to deleting.
If you wanted to automate this, then you could check that every box (or some critical subset of boxes) are checked. That way, to ignore the template's requirements yet pass the automation, you'd have to actively lie in your PR description. |
|
Incidentally, I have a personal checklist that I used to follow at my previous job, adapted to edX: https://gist.github.com/timmc-edx/03cc4bebd9d524c7cf5b7dc268f55d21 -- something to mine for ideas, but not applicable to all repos. |
|
|
||
| **Merge checklist:** | ||
| - [ ] All reviewers approved | ||
| - [ ] CI build is green | ||
| Check off if complete or not applicable: |
There was a problem hiding this comment.
Sorry, I thought of one more thing 😬
| Check off if complete or not applicable: | |
| Check off if complete or not applicable: | |
| - [ ] Backported to [latest and next named releases](https://docs.openedx.org/en/latest/community/release_notes/named_release_branches_and_tags.html?highlight=latest#latest-open-edx-release) |
IMO that would remove the need for BTR to stick this big warning at the top of the edx-platform PR template.
There was a problem hiding this comment.
Backports are usually only relevant for IDAs, right? And versioning and changelogs are usually only done for libraries. So maybe it makes sense to keep separate templates for the various cookiecutters after all.
There was a problem hiding this comment.
It's a PITA so we try to avoid it, but we do need to backport fixes to libraries sometimes: https://openedx.atlassian.net/wiki/spaces/COMM/pages/2065367719/Making+a+pull+request+for+a+named+release#Backporting-to-a-library-repository
But anyway, to completely contradict my own suggestion: maybe this is trying to hard? I'm realizing that any truly exhaustive list of pre-merge items would be too long for a good PR template. I think there's virtue in having one simple, universal PR template, rather than trying to build a bunch of templates for different types of repos.
In terms of versioning and changelogs: Can we phrase it in a way that works for IDAs too? Maybe something like:
- Release process followed
Where "release process" is a link to a wiki page that explains or links out to:
- our backend library release process (version bump, changelog, whatever)
- our frontend library release process (version bump, GH release, whatever)
- the process for MFEs and IDAs, for:
- core contributors (message relevant
#cc-*channel) - 2U engineers (could link to an internal page)
- core contributors (message relevant
- the backporting process & how to know whether it applies to your PR
I realize that building such a page would take some work...
There was a problem hiding this comment.
After more consideration, I think we actually want to go in the exact opposite direction: Customize each repo's PR template as much as possible. Some repos keep changelogs, others do not; some have versions, others are continuously released; for some, manual testing is appropriate, and for others it is not.... The more I tried to create a universal template, the larger it got.
Fundamentally, I think there are three critical principles in checklist design:
- Make it as short as possible -- otherwise people get annoyed/fatigued/intimidated
- Have zero irrelevant items for the vast majority of occasions -- otherwise people get in the habit of checking off items without considering them carefully
- Have the easiest-to-forget things highest in the list, to increase the chance they are noticed
I don't think you can create a universal template and also stick to those items, particularly # 2. In fact, not having the template be too uniform probably hurts # 3, as it increases a sense of familiarity, which reduces the chance people will stop and think.
Anyway, take a look at the latest. The commit message describes the differences between the variations. If you hate it, I can back it out, but I think this approach has the best chance of increasing participation.
There was a problem hiding this comment.
I agree with those checklist design principles. Once the checklists are designed, though, I think three things need to happen in order for them to be adopted:
- New repos get the correct PR template. This is easy enough.
- Existing repositories are updated to use correct PR template. (Yes, we could skip on updating existing repos. However, I don't see these checklists having an impact unless folks see them basically everywhere.)
- For an org-wide template, it'd be easy. Just delete all existing PR templates 💥
- For customized templates, you'd need to go in and update every repo to the right PR template.
- Checklists are updated as repos & processes change over time.
- For an org-wide template, this would just mean editing the central PR template or supplementary wiki pages.
- For customized templates, this would mean updating templates across repositories, every time things change. Either maintainers or a central team like Arch-BOM would need to take on this responsibility. (FWIW, tCRIL has some new automation for cross-repo updates. We've been using it to keep certain workflows consistent across repos. Still, merging all the generated PRs is annoying).
Personally, I think I would still choose an org-wide template just to avoid taking on the up-front cost of (2) and the maintenance burden of (3). That said, if customized templates seem worth the lift to you, then no opposition from me 👍🏻
There was a problem hiding this comment.
Yeah, I definitely feel the tension of "ugh, not another thing to keep updated", which I think I actually mentioned earlier. But PR templates are intimately tied to ownership, and different maintainers have different checklists (at least in their minds, if not explicitly written down.) It would actually be really cool if we could standardize maintainership, and then I think there'd be a really strong argument for standardized PR template checklists. But even then we'd have differences for libraries vs. IDAs, and GH doesn't have a way to say "I want Standard PR Template No. 5".
I'm OK with the idea of having to automate (or semi-automate) replacement of PR templates at some point. I think we're going to need to get better at that sort of thing eventually anyhow. One option would be to fully automate modification of PR templates that haven't been customized; that would be a pretty safe thing to do, since you could say "auto-merge any where the the diff looks exactly like this" and be confident that the automation wasn't screwing anything up. That's how I'd want to approach any such future change.
(EDIT: Adjusted wording: s/edited/customized/)
There was a problem hiding this comment.
Sounds good 👍
Once you merge I recommend linking up with the #maintainership-pilot folks. They have a draft repo standards list going and it sounds like "appropriate PR template" ought to be on there.
- Emphasize "or" in prelude - Remove conventional commits link - Add unit tests, manual testing; remove "testing instructions" from catchall - Prune irrelevant items by type: No version bump or changelog for IDA; no manual testing for library. app and xblock are identical, but probably should differ in some way. The repo's own template is adjusted to match app/xblock, but without version bump or migration note.
There was a problem hiding this comment.
Just a couple more nits. See my comment here, but if you do stick with the customized templates route, then I think these templates look great.
cookiecutter-django-ida/{{cookiecutter.repo_name}}/.github/PULL_REQUEST_TEMPLATE.md
Outdated
Show resolved
Hide resolved
cookiecutter-django-ida/{{cookiecutter.repo_name}}/.github/PULL_REQUEST_TEMPLATE.md
Outdated
Show resolved
Hide resolved
# Conflicts: # CHANGELOG.rst
I notice that people largely ignore PR templates, especially big ones.
I think that shrinking this down to just the essentials will increase the
likelihood that people use it.
In particular, I want to remove things like "All reviewers approved" and
"CI build is green" that are generally already enforced by repo policies,
"Delete working branch" (there's a setting for that if we want it!), and
release instructions (should be in a central document linked from the
README).
Merge checklist:
Check off if complete or not applicable: