GitHub labels and PR review workflow

(Carol Willing) #1

I’ve been triaging some open PRs, and I wonder if we should consider updating some of the labels to simplify triaging actionable PRs.

In other projects that I maintain, I rarely look at a PR that does not have a green checkmark (unless someone @ mentions me). Once the green check appears, I know that the PR is ready for review and subsequent merge if there are no review issues.

Currently, many PRs get a red X when there is no news or issue number or if the CLA needs signature. Yet, those same PRs get an “Awaiting …” label. Ideally, I would like to filter the PRs that are complete and actionable (ready to merge on positive review).

Perhaps “news” and “issue number” shouldn’t trigger the red X. Alternatively, waiting to apply any “Awaiting…” label until the tests and news, issue #, and CLA all pass may also be a good approach. Since we are tight on reviewing time, it increases our productivity to only flag for human review the PRs that have met all automated checks.

(Łukasz Langa) #2

I think we need to keep the red X. Before we were blocking on NEWS and CLA, we routinely got changes through that never followed up on either. Missing changes from NEWS sucks because it misleads the user to think there was no change when in fact there was one. Missing CLAs are dangerous, I think @tjreedy went on a hunt for those a while back.

But you’re right that we might want to wait with applying “Awaiting…” labels in those cases. And one of our bots could state what’s up:

“”"
Thanks for your change! Looks like you are missing a NEWS entry! Add one by using blurb: [LINK TO HOWTO]

Pro-tip: pull requests with a green checkmark get more attention.
“”"

(Carol Willing) #3

Makes sense re: NEWS and CLA and the red X.

Your suggestion to delay applying “Awaiting…” and suggested message would be :smile:

(Łukasz Langa) #4

We can probably leave the implementation of this to our Bot Queen when she’s back from her month off. What do you think, Carol?

(Carol Willing) #5

Absolutely. No rush. Perhaps she can teach me her bot magic too :jack_o_lantern:

P.S. Looking forward to her keynote at DjangoCon tomorrow too :tada:

(Jeroen Demeyer) #6

On the other hand, for very minor fixes like documentation fixes, you don’t need an issue number or NEWS entry. You don’t want people to artificially add NEWS entries like “fixed a typo in distutils documentation” just to get the green check.

(Łukasz Langa) #7

Yeah, that’s nothing new. We have “skip news” labels for that reason.

#8

When this happens, as core dev you can tell the contributor to remove the news entry, and you can add the skip news label.

(Jeroen Demeyer) #9

My question was really: what should the PR submitter do ideally? If he doesn’t add a NEWS entry, then (as @willingc said above) the PR might be ignored because of the red cross. And recall that non-core devs cannot add the “skip news” label.

(Brett Cannon) #10

All PRs need an initial once-over to make sure they are in the proper state. We could potentially make it more self-servicing to say “I don’t think this PR needs X” and then clearly mark the PEP with a label for that or simply let any user set the “skip” labels through a comment.

#11

I think this is good idea, so perhaps there should be an awaiting green checkmark (or better naming) before awaiting review

While this is possible (miss-islington already knows how) it was also causing ratelimit issue for miss-islington.
Checking if all tests have been run and completed for a single commit requires multiple API calls. :confused: (We have been using caching too).

I will have think of how this can be further optimized.

I guess this could be linked to the upcoming blurb_it project.