Contradictory Trac instructions on when to set "Has patch"

Django Internals
1

When contributing a new patch (as a PR) the instructions on the Trac ticket are (bold mine):

Check the “Has patch” flag on the ticket after sending a pull request and include a link to the pull request in the ticket comment when making that update. The usual format is …

Trac is saying to create the PR first and then set the Trac flag. In my experience though this immediately causes a warning comment on the PR:

:warning: Warning: Incorrect Trac Ticket Flag

At the time of making my first contribution this seemed at odds with the PR checklist template - “I have checked the “Has patch” ticket flag in the Trac system”. I took a punt on the day and got it wrong, so suspect this is catching out other new contributors as well.

Not sure there is any way to change the mechanics of the PR check to allow a grace period, so can we update the Trac instructions instead?

Set the “Has patch” flag on the ticket before sending the pull request. Once sent update the Trac comment with a link to your new pull request. The usual format is …

4 Likes
2

I got tripped up with the same thing too - #11 in my messy public notes on the first contribution instructions

1 Like
3

Great write up @StephanieAG. Any improvements to suggest for the new wording? For comparison:

Set the “Has patch” flag on the ticket before sending the pull request. Once sent, Edit your “Has patch” comment in Trac to include a link to the new pull request. The usual format is …

Using “Edit” to match the UI and more clearly guiding the user to update their comment rather than create an additional one. Ends up looking something like this: #17752 (Serialization and multi-table inheritance) – Django

Better to less specific / potentially brittle?

4

This would be my strong preference if possible.

I worry that—in our attempt to thwart AI sloppers and stat gamers—we’re making things too complex for genuinely well-intentioned new contributors. Instantly closing a PR on a correctable technicality like “didn’t check has-patch yet” seems aggressive.[1] Particularly if the opposite order seems to make more sense to you (:raised_hand:).

We might borrow some ideas from CPython. Rather than closing PRs immediately, their gang of bots notes any problems and waits to apply the “ready for review” label until they are fixed. Reviewers ignore PRs that aren’t marked ready. (I’m not sure if they’ve automated closing PRs that don’t get corrected, but that’s an entirely reasonable use for a stalebot: our review bot made a specific request, N days have elapsed without it being addressed, now we’re closing the PR.)

  1. I’m on a bit of a crusade against automated processes that exhibit a “computer says no” attitude. Even more so of late: we need to model the behavior we’d want from our future AI overlords benefactors, now, while they’re still willing to learn from us. :face_with_diagonal_mouth: ↩︎

1 Like