Bot checks for typo PRs (some considerations)

[StephanieAG]

I came across a typo in the docs so I tried my first PR for Django.

The bot closed the PR, and thanks to the helpful/clear error messages, I understand why. I can resubmit so that it doesn’t automatically close it but mentioning here because the things that are being flagged are:

• my branch description isn’t long enough (“Grammar fix” didn’t meet a 5 word requirement that I didn’t know about)
• my branch description kept the placeholder text (which I wasn’t sure if I should keep or remove)
• I wrote N/A for checklist items that don’t apply to typos when the bot check (understandably) wants all of the checklist items marked completed

I totally see why these are great bot checks generally speaking. Just tossing out a few minor adjustments for consideration:

• adding a comment under Branch description heading (just like there is under the Trac ticket number heading) that says something like <!-- Replace this placeholder text with a branch description of at least 5 words. -->
  • For context, when I was submitting, I wasn’t sure if I was supposed to delete that template line or not so I kept it because the Trac ticket one explicitly told me to replace or delete whereas the branch description one did not. (In retrospect, it obviously makes more sense to delete the line because no one reviewing would need to see a template line like that.)
• either make “N/A” okay to have for the checklist items that don’t make sense for typos (e.g. “I have checked the “Has patch” ticket flag in the Trac system.”) or add a comment that says something like <!-- For typo fixes, you can mark checklist items as completed if they are not applicable. -->. Other options could be to have two versions of the checklist or section out the one checklist into two parts (the main part encompassing all PRs and the second part just for anything more than a typo). Personally, I think the comment solution would be best because it seems simplest yet still effective.
• One other recommendation which doesn’t matter for the flow, but I’ll mention since I have my feedback hat on: in the auto comment listing the errors, I’d consider replacing the emoji with something something like or or or or or similar.
  • I think this would be helpful because I found the to be kind of aggressive/alarming/jarring but also thought it was useful in that it provides a bright bullet point for the thing/s that went wrong. To elaborate, it felt a little like rubbing salt in the wound for the mistakes - like, the PR is already closed by the bot and it says “Error” right there (in my case, more than once lol) so the recipient already has very clear feedback that “this contribution was problematic for these reasons.” The stop sign shape also kind of subconsciously implies “STOP [contributing]” even though of course anyone who thinks for a second would understand it’s more meant like “STOP there are these things to fix before resubmitting.” I think one of the alternate emojis would lend itself better to the second meaning.
  • It is possible I am over thinking this since I know I generally lean a bit more on the sensitive side () and maybe others don’t feel quite so alarmed when looking at , but I’ll still throw this out there, especially since new contributors would naturally be the most likely people to encounter these errors and (I imagine) are also more sensitive to negative feedback than experienced folks, so maybe being extra sensitive in this detail could leave a new contributor feeling more encouraged like “okay, I made some mistakes, but it’s helping flag for me what to improve” as opposed to feeling more like “I was stopped because I did wrong”

Anyway, just some thoughts on my first PR experience. Grain of salt for the rec’s, take 'em or leave 'em

[CodenameTim]

Thanks @StephanieAG, I think you’re highlighting reasonable improvements.

What do you think about moving the “Provide a concise overview of the issue or rationale behind the proposed changes.” into a comment, <!-- Provide a concise overview of the issue or rationale behind the proposed changes. -->?

@StephanieAG

either make “N/A” okay to have for the checklist items that don’t make sense for typos (e.g. “I have checked the “Has patch” ticket flag in the Trac system.”) or add a comment that says something like <!-- For typo fixes, you can mark checklist items as completed if they are not applicable. -->

I suspect having the user either leave them as unchecked or checked would be preferable from a code perspective than allowing N/A. @frankwiles thoughts?

@StephanieAG

The stop sign shape also kind of subconsciously implies “STOP [contributing]” even though of course anyone who thinks for a second would understand it’s more meant like “STOP there are these things to fix before resubmitting.” I think one of the alternate emojis would lend itself better to the second meaning.

We are using the for the warning type messages for concerns that don’t cause a PR to be closed, so to me that eliminates the possibility of using the orange and yellow items. How would you feel about ? Alternatively, we could remove the emojis entirely?

[blighj]

For the record only the first 5 items need to be checked. So in your case it was editing this one that caused that flag.

The commit message is written in past tense, mentions the ticket number, and ends with a period (see guidelines).

I can see the confusion because it says … “mentions the ticket number,”… In practice once you have done the other things covered, it is okay to check it rather than edit it. Maybe the team could add an (if applicable) to that wording to help make it clearer.

The other four checklist options are not checked by the bot and can be edited to say N/A or ignored as the contributor wishes. An if applicable header could be added there too, but there is a tradeoff on clarity and verbosity. I’d probably favour leaving it as is, but recognise that might be familiarity bias

[frankwiles]

Yeah you just need to leave them unchecked. However, one thing I just noticed @nessita @jacobtylerwalls @sarahboyce is that in the conversion from my example repo to the Django repo we seem to have lost the exception for docs/* only changes which would have let this also sail through smoothly.

Not sure if that was a mistake or on purpose…

[jacobtylerwalls]

we seem to have lost the exception for docs/* only changes which would have let this also sail through smoothly.

I can’t find the discussion right now, but for me, it’s on purpose – I don’t think docs PRs deserve special treatment.

Historically, we’ve been very open to typo fixes. I’m okay pausing work on typo fixes until we can improve the quality bot (detecting that something is a typo fix is a stretch goal).

@StephanieAG thanks for your pull request and for your thoughtful feedback!

I would also like to replace the stop sign emoji.

adding a comment under Branch description heading (just like there is under the Trac ticket number heading) that says something like <!-- Replace this placeholder text with a branch description of at least 5 words. -->

+1!

[medmunds]

@StephanieAG

either make “N/A” okay to have for the checklist items that don’t make sense for typos (e.g. “I have checked the “Has patch” ticket flag in the Trac system.”) or add a comment that says something like <!-- For typo fixes, you can mark checklist items as completed if they are not applicable. -->.

I regularly change the task checkboxes to “n/a” for items that don’t apply. It feels wrong to check “I have added or updated relevant tests” for a PR that doesn’t touch the tests (and doesn’t need to). But leaving that unchecked makes me think I still have work to do. We might solve that by adding “if applicable” in the template for all checkboxes where it, um, applies.

(Related aside: it bugs me, just a little, that the template uses task checkboxes for the AI disclosure radio buttons, so the best a PR can score is complete.)

[jacobtylerwalls]

@medmunds

We might solve that by adding “if applicable” in the template for all checkboxes where it, um, applies.

I’ve had this thought also!

[StephanieAG]

What do you think about moving the “Provide a concise overview of the issue or rationale behind the proposed changes.” into a comment, <!-- Provide a concise overview of the issue or rationale behind the proposed changes. -->?

@CodenameTim Love it, I think it’s more intuitive and simpler than my suggestion.

Not sure if the 5 word minimum is a bot requirement or just what the error message recommends when a branch description fails (I assumed the former originally, but my other assumption about what was going on was wrong so maybe that’s the case here too). If the word count is a bot requirement, maybe altering the comment text to say something to indicate that?

I’m definitely talking about things I don’t know here so major grain of salt here: alternatively, maybe consider removing/altering the 5 word requirement if it is there? Thinking that 2 vs 5 words doesn’t really change much for description for humans, e.g. I said “Grammar fix” but to pass the check I would’ve had to be more verbose which I don’t think really gives more info with the extra few words. For the PRs that aren’t just typos, I imagine it’s unlikely that a helpful branch description would be under 5 words anyway so the minimum (seemingly) still doesn’t provide benefits. Not sure if that makes sense: in other words, I imagine a helpful branch description would either naturally hit a 5 word minimum without even trying or, if the check passes at 4 words, I’m skeptical an extra 2 words would’ve really have made that description that much more helpful. Also, for non-human submissions, I don’t think there’s a high likelihood that AI would submit anything under 5 words since it generally leans verbose. Anyway, I don’t really know about this in practice, so I’ll obviously defer to people who actually know what happens.

How would you feel about ? Alternatively, we could remove the emojis entirely?

@CodenameTim No emoji is okay, though I like that they broke up the text into sections more obviously. is better than and I’ll take it if there must be red (and makes sense re: warning/yellow precedent). Maybe , ,,, or similar neutral but still eye-catching?

For the record only the first 5 items need to be checked. So in your case it was editing this one that caused that flag.

@blighj ohhhh lol thanks for clarifying re:

only the first 5 items need to be checked

I was very much thinking like @medmunds described below when I saw the checklist (with the additional thought that a PR wouldn’t be accepted for review if the checklist requirements weren’t met):

It feels wrong to check “I have added or updated relevant tests” for a PR that doesn’t touch the tests (and doesn’t need to). But leaving that unchecked makes me think I still have work to do.

[CodenameTim]

@StephanieAG I think the minimum number of words for the description is going to be arbitrary to some degree. For now, I think we should keep it as is and see how things pan out over the next few months. I think we’ll want to review the auto-closed PRs to see which parts are properly catching sloppy PRs and which are causing unnecessary turmoil, but we need some more data first.

I suspect if you open a Trac ticket for the emoji change, moving the branch description to a comment and a clarification on the checkboxes, it would be accepted. That said, it may be prudent to wait a few more days though to allow others to chime in with opinions.

[nessita]

@frankwiles

the conversion from my example repo to the Django repo we seem to have lost the exception for docs/* only changes which would have let this also sail through smoothly.

This was very intentional, docs changes bigger than typo fixes should also have an accepted ticket. Otherwise, we get a flood of “rewrites” built by LLMs which in practice add no value to the docs and waste maintainers (and CI) time.

[nathanielx044-gif]

Yeah, I had a similar first PR experience.

The bot feedback is actually helpful, but a couple of things like branch description length and placeholder text aren’t very clear for first-timers. Also, the checklist part can feel a bit confusing for simple typo fixes.

And I agree on the emoji point too the stop sign feels a bit harsh even when the feedback is valid. A softer warning icon would feel more friendly.

Overall though, the error messages do help you fix things and try again easily.

[StephanieAG]

@CodenameTim

open a Trac ticket

Okay, done: #37093
Kind of felt like two separate things but wasn’t sure about separating so kept them together anyway but separated them within the ticket

Related aside: it bugs me, just a little, that the template uses task checkboxes for the AI disclosure radio buttons, so the best a PR can score is complete.

@medmunds just wanted you to know that I saw your feedback above and agree that would bug me a little too, but I didn’t know how best to address it so I left it out (like, I don’t think there is a radio button option sort of possibility here and couldn’t think of a different alternative that could be incorporated)