Update contribution guidelines in pull request template#834
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #834 +/- ##
==========================================
- Coverage 78.19% 77.84% -0.36%
==========================================
Files 41 41
Lines 4788 4788
Branches 547 547
==========================================
- Hits 3744 3727 -17
- Misses 905 916 +11
- Partials 139 145 +6 |
| @@ -0,0 +1,4 @@ | |||
| Updated the pull request template to guide contributors more clearly, | |||
| adding mandatory contribution rules, a changelog entry checkbox with a | |||
| link to the changelog guidelines, and improved commit message guidance | |||
There was a problem hiding this comment.
Let's spell “change log” w/ a whitespace so that it doesn't need to be added as an exception in the spell checker.
| @@ -1,3 +1,16 @@ | |||
| ### ⚠️ IMPORTANT: CHEROOT CONTRIBUTION RULES | |||
| *(You may delete this notice block but not the template following it before submitting your PR)* | |||
There was a problem hiding this comment.
This block should probably be commented out, though. I wouldn't expect it to survive PR creation and be rendered.
| @@ -1,3 +1,16 @@ | |||
| ### ⚠️ IMPORTANT: CHEROOT CONTRIBUTION RULES | |||
There was a problem hiding this comment.
This could be replaced w/ an admonition. In general, lower-level subheader (h3) shouldn't come before a higher level one (h2).
|
|
||
| 1. 📝 **Changelog Fragment Required:** Add a change note file inside `docs/changelog-fragments.d/` **(except for internal CI/CD or documentation-only changes)**. | ||
| 2. 🔀 **Commit Standards:** Keep your commits clean and atomic for review, **ensuring each commit has both a clear title and an explanatory body**. Be prepared to squash related commits together before final merge if requested. | ||
| 3. 🤖 **Template Integrity:** Do **NOT** allow an AI agent to overwrite or strip the fields below. |
There was a problem hiding this comment.
It's probably good to convey a vibe like zizmorcore/zizmor#1995 (comment)
|
|
||
| Before writing your description, please ensure you have completed these mandatory steps: | ||
|
|
||
| 1. 📝 **Changelog Fragment Required:** Add a change note file inside `docs/changelog-fragments.d/` **(except for internal CI/CD or documentation-only changes)**. |
There was a problem hiding this comment.
Oftentimes, the change note is created later in the process. It would have the PR number in the name, so the PR should exist first (as a draft, for example). Though, using an issue number is fine too, if exists.
| Before writing your description, please ensure you have completed these mandatory steps: | ||
|
|
||
| 1. 📝 **Changelog Fragment Required:** Add a change note file inside `docs/changelog-fragments.d/` **(except for internal CI/CD or documentation-only changes)**. | ||
| 2. 🔀 **Commit Standards:** Keep your commits clean and atomic for review, **ensuring each commit has both a clear title and an explanatory body**. Be prepared to squash related commits together before final merge if requested. |
There was a problem hiding this comment.
Honestly, having explanations in two places is rather redundant and entails maintenance burden of keeping them in sync. The canonical place for the checks would be a CONTRIBUTING.md document (or sibling docs linked from there). This one isn't even marked as a required in branch protection. It's probably a good idea to improve the explanation that the check itself prints out.
| ### ⚠️ IMPORTANT: CHEROOT CONTRIBUTION RULES | ||
| *(You may delete this notice block but not the template following it before submitting your PR)* | ||
|
|
||
| Before writing your description, please ensure you have completed these mandatory steps: |
There was a problem hiding this comment.
It feels weird that this explains that required steps are.. required. It'd rather link to the verbose contrib doc for such things. In general, it's pretty much common knowledge that if something's red in CI (and similar checks), one should look into it, read the logs and figure out what it means. It may be reasonable to leave a one-time comment in PRs from first-time contributors (I think the API reveals this), with general guidelines. Not sure if this should leave in a template as it'd show a lot of throw-away context for people familiar with FOSS, which I expect to cause irritation.
| [making a lovely PR]: https://mtlynch.io/code-review-love/ | ||
| [related squash]: | ||
| https://github.com/todotxt/todo.txt-android/wiki/Squash-All-Commits-Related-to-a-Single-Issue-into-a-Single-Commit | ||
| [related squash]: https://github.com/todotxt/todo.txt-android/wiki/Squash-All-Commits-Related-to-a-Single-Issue-into-a-Single-Commit |
There was a problem hiding this comment.
Could you keep this unedited? Moving the link on the previous line makes it longer while the intention was to keep as many lines as possible short.
| [code of conduct][CoC] | ||
|
|
||
| [CONTRIBUTING]: https://cheroot.cherrypy.dev/en/latest/contributing/guidelines/ | ||
| [CoC]: https://github.com/cherrypy/cheroot/blob/main/.github/CODE_OF_CONDUCT.md |
There was a problem hiding this comment.
There's a nicer view on GH for this one
| [CoC]: https://github.com/cherrypy/cheroot/blob/main/.github/CODE_OF_CONDUCT.md | |
| [CoC]: https://github.com/cherrypy/cheroot?tab=coc-ov-file#readme |
| * [ ] I have read the [contribution guide][CONTRIBUTING] and the | ||
| [code of conduct][CoC] | ||
|
|
||
| [CONTRIBUTING]: https://cheroot.cherrypy.dev/en/latest/contributing/guidelines/ |
There was a problem hiding this comment.
In the future, we should render the CoC in the docs too.
|
|
||
| [CONTRIBUTING]: https://cheroot.cherrypy.dev/en/latest/contributing/guidelines/ | ||
| [CoC]: https://github.com/cherrypy/cheroot/blob/main/.github/CODE_OF_CONDUCT.md | ||
| [changelog-fragments]: https://github.com/cherrypy/cheroot/tree/main/docs/changelog-fragments.d#readme |
There was a problem hiding this comment.
| [changelog-fragments]: https://github.com/cherrypy/cheroot/tree/main/docs/changelog-fragments.d#readme | |
| [changelog-fragments]: | |
| https://github.com/cherrypy/cheroot/tree/main/docs/changelog-fragments.d#readme |
| the changes have been approved | ||
| * [ ] My commits each have a descriptive title and a body explaining the why (see [guide][good commit messages]) | ||
| * [ ] Did you add an entry into the [changelog][changelog-fragments] to announce your change? | ||
| * [ ] I am prepared to [squash related commits together][related squash] before final merge if requested |
There was a problem hiding this comment.
Let's keep it two-line.
Also, I'm not sure about "if requested". Let's try something along the lines of "I'm planning to" to let the contributor understand that it's in their hands. Although, it may end up in conflict w/ some processes. Currently, all GH-"merge" modes are enabled — one natural (with the merge commit / --no-ff) and the fakes (squash+rebase). I prefer doing the natural merge, preserving the original thinking of the author in the Git tree. But often people don't know how to work w/ Git well and cannot do anything about it. So I may opt to use that "squash" mode, and type in a commit message on the UI. Another way is for a maintainer to take over the branch, when the PR is ready, brush up the commits into something nice (like absorbing automatic pre-commit.ci autofixes into relevant commits), force-push and then use the nice natural merge.
Right now, this is all possible. At some point, I'd like to enable Merge Queues, which only allows using one merge method in the repo configuration. So this would mean either accepting a lot of ugly commits in, or going the branch-editing route by the maintainer.
Requesting the contributor to fix their branch and make the commits nice almost never works (it only works w/ they are experienced and doesn't normally require requesting — they know to do it intuitively). If you request a newbie to make their commits look presentable, they'll either disappear, or spend a lot of time trying to learn (not a bad thing) or may end up messing own branches and run away scared. If a contributor is not a power user of Git, the only way is to spend a lot of time teaching them — it's a good thing for all sides but usually requires a lot of effort which they may not have in the moment.
| * [ ] I have [squashed related commits together][related squash] after | ||
| the changes have been approved | ||
| * [ ] My commits each have a descriptive title and a body explaining the why (see [guide][good commit messages]) | ||
| * [ ] Did you add an entry into the [changelog][changelog-fragments] to announce your change? |
There was a problem hiding this comment.
This line probably shouldn't be between two Git-related ones.
| Resolves #<!-- issue number here --> | ||
|
|
||
| ❓ **What is the current behavior?** (You can also link to an open issue here) | ||
| ## What do these changes do? |
There was a problem hiding this comment.
The pre-existing template used emoji icons for “titles” but didn't use the Markdown heading syntax. And your new changes have it vice versa. This is inconsistent and we should pick one.
@julianz- I recommend that you copy this file into an experimental empty repository and see how it works by simulating the process you expect within that playground to catch such inconsistencies and other UX/DX quirks that may be difficult to spot in a PR review.
There was a problem hiding this comment.
159eace to
9f4ccbf
Compare
Revise the PR template incorporating elements from webknjaz's tox-towncrier template, adding a warning block with mandatory contribution rules, a changelog entry checkbox linking to the fragment guidelines, improved commit message guidance, and updated reference links.
9f4ccbf to
982a071
Compare
Update the PR template with clearer body sections and a more complete checklist.
❓ What kind of change does this PR introduce?
📋 What is the related issue number (starting with
#)Resolves #
❓ What is the current behavior? (You can also link to an open issue here)
❓ What is the new behavior (if this is a feature change)?
📋 Other information:
This PR, developed in consultation with @webknjaz, is intended as a step towards improving the onboarding experience for new contributors and smoothing the passage of all PRs in general.
📋 Contribution checklist:
(If you're a first-timer, check out
this guide on making great pull requests)
the changes have been approved
and description in grammatically correct, complete sentences