Core-workflow: Mark PRs based on their stage of review

[brettcannon]

The idea is to make it so that it's very clear to a core dev waiting for review comments to be addressed that the PR is ready for another round of review. Probably something along the lines of when a core dev leaves a "Changes Requested" review that a label is applied like "WIP" and then a comment left for the PR submitter saying that they should send a message to the reviewer like @brettcannon ready for review (changing the title was shot down by Guido at the PyCon US 2017 language summit). That way the reviewer gets a mention notification and the bot can then remove the label. This should lead to the PR showing up in GitHub's notification in a visible way that it's ready for another round of reviews.

I'm not sure if a label signaling another round of review is necessary as the absence of the "WIP" label is a visual signal of that. But perhaps an explicitly "waiting for review" label would also be helpful? This could also be automatically applied to all new PRs to signal when a PR is awaiting an initial review (or an alternative "initial review needed" label?). I also don't know how external reviews play into this, e.g. do we have "need core review" when an external person left a review but a core dev has yet to review?

[brettcannon]

I've been thinking about this and the stages might be:

• awaiting 1st review: no one has reviewed the PR yet
• awaiting core review: someone has reviewed the PR who isn't a core dev, and now a core dev should have a look
• awaiting changes: a core dev left a review and the PR is now blocked on the PR submitter doing some work
• awaiting another review/awaiting followup review/awaiting re-review/awaiting nth review: the PR submitter believes they have addressed all the reviewer comments and is ready for another look

Once a core dev has done an initial review the PR will end up cycling between awaiting changes and awaiting another review/whatever-the-label-is-named until the PR is closed.

All of these labels can be automatically applied in various ways, so keeping them up-to-date won't be a burden.

[brettcannon]

Potential naming schemes:

• awaiting first review
• blocked-first review
• blocked - first review
• blocked on first review
• blocker-first review
• blocker - first review
• stage-first review
• stage - first review
• waiting on first review

[brettcannon]

https://github.com/python/cpython/pull/2015 is an example of a PR where a non-core dev provided a review.

[brettcannon]

I'm really tempted to have the message that PR submitters say in a message is "I didn't expect the Spanish Inquisition!". 😆 If we decide to have Bedevere reply then we could have it say something like "Nobody expects the Spanish Inquisition! (@brettcannon)" to guarantee people know they left a review that is has been addressed.

[terryjreedy]

I strongly prefer 'awaiting' (or secondarily the synomyn 'wait for') to 'blocked'. The action implication may be the same, but the emotive connotation is not. There is a difference between a robot blocking authors versus helping them by letting them know what needs to happen ;-).

I am not sure about the multiple stages. There is a point at which 'help' can become 'busywork'. There is already a '(blocked) awaiting response to core-dev review stage'.

Let's make sure we do not belittle reviews from non-core devs. They can be very valuable.

I don't understand the Spanish Inquisition comment. The sentences do not scan very well.

[brettcannon]

@terryjreedy all of this will be automated except for when a PR creator wants to signal that they are done addressing the current reviewer comments and is ready.

And I think by making an external review count for something we are not belittling anyone. But I'm also not willing to trust someone who suddenly decided to contribute a review to know what to look out for and thus consider their review good enough to signal that a PR is ready to go.

And the Spanish Inquisition quotes provide two things. One is it gives the PR creator an unambiguous way to signal that they are ready to have their PR reviewed again because they believe they have addressed all review comments. Two, the response message gives people who use email with GitHub something easy to filter on to get clear notification that a PR that they left a "request changes" review is ready for another look. (This topic is being actively discussed on the core-workflow ML.)

[brettcannon]

Here is a dot graph showing what I'm thinking the stages should be and how to transition between them (you can visit http://www.webgraphviz.com/ to view the graph online).

digraph "PR stages" {
  "New PR" [color=blue]
  "Awaiting review" [shape=box, color=orange]
  "Awaiting core review" [shape=box, color=green]
  "Awaiting changes" [shape=box, color=blue]
  "Awaiting change review" [shape=box, color=green]
  "Awaiting merge" [shape=box, color=green]

  "New PR" -> "Awaiting review" [label="PR by non-core dev", color=blue]
  "Awaiting review" -> "Awaiting core review" [label="Non-core review", color=orange]
  "Awaiting core review" -> "Awaiting changes" [label="Core dev requests changes", color=green]
  "Awaiting changes" -> "Awaiting change review" [label="PR creator addresses changes", color=blue]
  "Awaiting change review" -> "Awaiting changes" [label="Core dev requests changes", color=green]
  "Awaiting change review" -> "Awaiting merge" [label="Core dev approves PR", color=green]


  "Awaiting review" -> "Awaiting merge" [label="Core dev approves PR", color=green]
  "Awaiting review" -> "Awaiting changes" [label="Core dev requests changes", color=green]
  "Awaiting core review" -> "Awaiting merge" [label="Core dev approves PR", color=green]

  "New PR" -> "Awaiting merge" [label="PR by core dev", color=green]
}

[brettcannon]

Another key thing will be to leave HTML comments in any comments left by Bedevere to help mark when an event happened (e.g. when the request for changes was made). This way if we ever automate closing PRs that have stalled we can easily keep track of when things last happened.

[brettcannon]

I've started work on this in https://github.com/brettcannon/bedevere/tree/stage.

[brettcannon]

On python-committers, @haypo had an idea of dismissing code reviews if someone pushes a change post-review. Now GitHub can do this automatically, but it requires turning on mandated code reviews. If we were going to do this sort of thing ourselves then we would need to verify:

1. Dismissing a review via the API doesn't require the review mandate setting on
2. Review comments and such are not lost when a review is dismissed
3. Dismissed reviews still show up in the review history so that we can tell a PR has been previously reviewed versus starting from scratch and thus bask to "awaiting review"
4. We are okay with reviews being dismissed if we make a single change through the web UI as maintainers

[brettcannon]

This has now been merged! Just waiting for the first PR to trigger it before I email python-dev with an explanation.