Opened 2 days ago
Last modified 36 hours ago
#37093 new Cleanup/optimization
Adjust GitHub PR description template + bot check error emoji
| Reported by: | Stephanie Goulet | Owned by: | |
|---|---|---|---|
| Component: | Core (Other) | Version: | 6.0 |
| Severity: | Normal | Keywords: | PR, GitHub |
| Cc: | Tim Schilling | Triage Stage: | Accepted |
| Has patch: | no | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description
The below suggestions are based on the forum discussion, Bot checks for typo PRs (some considerations).
Suggestion 1: Adjust GitHub PR description template
When opening my first PR for a typo fix, I ran into a little confusion which resulted in an auto-closed PR that might've been avoided with some small adjustments.
I propose the following edits to the PR description template to improve clarity, particularly for new contributors.
The full current and proposed templates are below, however for convenience, you can see the differences more easily in this saved diff.
Current template:
#### Trac ticket number <!-- Replace XXXXX with the corresponding Trac ticket number. --> <!-- Or delete the line and write "N/A - typo" for typo fixes. --> ticket-XXXXX #### Branch description Provide a concise overview of the issue or rationale behind the proposed changes. #### AI Assistance Disclosure (REQUIRED) <!-- Please select exactly ONE of the following: --> - [ ] **No AI tools were used** in preparing this PR. - [ ] **If AI tools were used**, I have disclosed which ones, and fully reviewed and verified their output. #### Checklist - [ ] This PR follows the [contribution guidelines](https://docs.djangoproject.com/en/stable/internals/contributing/writing-code/submitting-patches/). - [ ] This PR **does not** disclose a security vulnerability (see [vulnerability reporting](https://docs.djangoproject.com/en/stable/internals/security/)). - [ ] This PR targets the `main` branch. <!-- Backports will be evaluated and done by mergers, when necessary. --> - [ ] The commit message is written in past tense, mentions the ticket number, and ends with a period (see [guidelines](https://docs.djangoproject.com/en/dev/internals/contributing/committing-code/#committing-guidelines)). - [ ] I have not requested, and will not request, an automated AI review for this PR. <!-- You are welcome to do so in your own fork. --> - [ ] I have checked the "Has patch" ticket flag in the Trac system. - [ ] I have added or updated relevant tests. - [ ] I have added or updated relevant docs, including release notes if applicable. - [ ] I have attached screenshots in both light and dark modes for any UI changes.
Proposed template:
#### Trac ticket number <!-- Replace XXXXX with the corresponding Trac ticket number. --> <!-- Or delete the line and write "N/A - typo" for typo fixes. --> ticket-XXXXX #### Branch description <!-- Provide a concise overview of the issue or rationale behind the proposed changes. 5 word minimum. --> #### AI Assistance Disclosure (REQUIRED) <!-- Please select exactly ONE of the following: --> - [ ] **No AI tools were used** in preparing this PR. - [ ] **If AI tools were used**, I have disclosed which ones, and fully reviewed and verified their output. #### Checklist <!-- Please leave non-applicable items unchecked. --> - [ ] This PR follows the [contribution guidelines](https://docs.djangoproject.com/en/stable/internals/contributing/writing-code/submitting-patches/). - [ ] This PR **does not** disclose a security vulnerability (see [vulnerability reporting](https://docs.djangoproject.com/en/stable/internals/security/)). - [ ] This PR targets the `main` branch. <!-- Backports will be evaluated and done by mergers, when necessary. --> - [ ] The commit message is written in past tense, mentions the ticket number (if applicable), and ends with a period (see [guidelines](https://docs.djangoproject.com/en/dev/internals/contributing/committing-code/#committing-guidelines)). - [ ] I have not requested, and will not request, an automated AI review for this PR. <!-- You are welcome to do so in your own fork. --> - [ ] I have checked the "Has patch" ticket flag in the Trac system. - [ ] I have added or updated relevant tests. - [ ] I have added or updated relevant docs, including release notes if applicable. - [ ] I have attached screenshots in both light and dark modes for any UI changes.
Suggestion 2: Remove the stop sign emojis from the bot check error messages
I propose replacing the :stop_sign: emoji that is shown in the PR bot check error messages with the :right_arrow: emoji to keep the list of errors eye-catching while neutralizing the color so as to reduce the likelihood for contributors (especially new contributors) to read the bright red stop sign as a little harsh. I believe the inclusion of "Error" before each error description as well as the fact that the PR is automatically closed is sufficiently clear to communicate that those errors need to be addressed for the PR to be reconsidered.
Alternatively, if the specific emoji replacement of :right_arrow: is undesired, I propose removing the :stop_sign: emoji without a replacement for the same reasons as above. This alternative might be more appropriate if we highly value errors only ever being exclusively associated with the color red, but still see value in reducing the harshness factor. The trade off is that this loses the eye-catching factor of having an emoji next to the error titles which is particularly helpful when first seeing the auto response because the list is automatically expanded (rightfully so, in my opinion) despite each error header being expandable/collapsible.
For the record, I'm also open to other emoji options as an alternative to the stop sign but tried to pick the one I thought was best keeping in mind the feedback in the forum comments (e.g. staying away from yellow/orange, etc.) to make this a clearer proposed edit to review.
See this bot-closed PR comment as an example of where the stop sign emojis appear.
Change History (4)
comment:1 by , 2 days ago
| Cc: | added |
|---|
follow-ups: 3 4 comment:2 by , 2 days ago
| Component: | Uncategorized → Core (Other) |
|---|---|
| Triage Stage: | Unreviewed → Accepted |
comment:3 by , 2 days ago
Replying to Jacob Walls:
A note: the automated PR quality check expects only the last two checkboxes to be optional, so I would suggest making that explicit in the checklist somehow.
My understanding was that the bot is checking the first five are checked, which would make it the last four, not the last two, that the bot sees as optional.
I'm open to being corrected on that, but it looks right from the code and those last four being if applicable makes sense
comment:4 by , 36 hours ago
Replying to Jacob Walls:
Are you interested in submitting a PR?
Sure, I can try
Based on what's been said so far: Updated V2 of draft edits.
- Added as a comment but before the last four items instead of two (blighj seems right to me but let me know if I'm misunderstanding anything)
- Also removed the "please" in the AI section for consistency across all the comment instructions
Had another thought which is one thing that moving the branch description to a comment does is make it so the comment will remain in submitted PRs. This is consistent with other sections having comments so maybe it doesn't matter, but if the reviewers care to keep it as not having a comment in the branch description for submitted PRs, an alternative edit could be:
Update the section to be:
#### Branch description Replace this text with a concise overview of the issue or rationale behind the proposed changes (5 word minimum).
Which would also require updating the branch description check just so that the placeholder text is matching against the new version of the placeholder text.
Is there a preference from reviewers' perspectives?
Thanks for the thoughtful ticket and suggested edits. Accepting.
A note: the automated PR quality check expects only the last two checkboxes to be optional, so I would suggest making that explicit in the checklist somehow.
Just before those two items, maybe:
"Leave the following items unchecked if not applicable:"
Are you interested in submitting a PR?