OK, here is a dot graph that lays out the stages and the flow between them
based on what has been discussed so far. You can use
http://www.webgraphviz.com/ to view the graph in a browser. Anything green
requires/blocks on a core dev, blue requires/blocks on the PR creator, and
orange is open to anyone.
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 created 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]
On Mon, 19 Jun 2017 at 05:08 Nick Coghlan <ncogh...@gmail.com> wrote:
> On 19 June 2017 at 02:36, Brett Cannon <br...@python.org> wrote:
> > On Sat, 17 Jun 2017 at 18:11 Nick Coghlan <ncogh...@gmail.com> wrote:
> >> On 18 June 2017 at 05:07, Brett Cannon <br...@python.org> wrote:
> >> > The stages I see are:
> >> >
> >> > 1. Awaiting first review: no one has reviewed the PR
> >>
> >> I'd just make this "Awaiting review",
> >
> > +1 to the simplification of the name.
> >> and have it advance only for a
> >> positive review with no negative reviews.
> >
> > But wouldn't that lead to more than one stage being set since this would
> > perpetually be set until you were ready to merge? As I said later, I
> don't
> > know if we want to gate on a non-core devs approval as it might not be
> > reasonable (e.g. we have had some non-core devs be extremely picky about
> PEP
> > 8 conformance).
>
> I'm also fine with having mixed reviews mean "it's a core dev's problem
> now".
>
> >> I think this is worth having for the reason you give - anyone can
> >> advance the state to "awaiting core review", and it gives a visual
> >> indication of "at least 2 humans think this is a reasonable patch" for
> >> core developers looking to pick up pre-filtered proposals.
> >
> > Don't you mean "one other person" based on the advancement from "awaiting
> > review"?
>
> 2 humans - the original submitter, and whoever did the initial review
> (if they weren't a core dev).
>
> > Sorry if I wasn't clear before, but what I was thinking was that if
> > a core dev did the initial review then this stage would be skipped. IOW a
> > single core dev review is all that's needed to get out of any "I need a
> > review" stage.
>
> Right, that's what I was assuming as well.
>
> I also guess that when the submitter *is* a core dev, we'd send a PR
> straight to "awaiting merge" unless we had already explicitly added
> the "awaiting changes" or "awaiting core review" label.
>
> > If you do mean you want to require two reviewers for all PRs, I'm not
> sure
> > if we are there yet.
>
> No, I didn't mean that (although I can see how you read it that way) -
> I still don't think we should even make reviews mandatory for core dev
> submitted PRs.
>
> While I do think it would be nice to have the luxury of being able to
> afford to implement such a policy, it's the kind of workflow that's
> really only feasible when you have multiple folks handling reviews on
> a regular basis as part of a full-time job (and even then it can cause
> problems in niche areas of a code base where nobody feels particularly
> well-equipped to carry out an authoritative review).
>
> >> +1 for only getting here based on negative core reviews. An
> >> alternative to requiring a special comment to exit this state would be
> >> to add a "WIP: " prefix to the PR title when the label is added - that
> >> way the submitter can just remove the prefix to indicate that they
> >> have made the requested changes and are ready for another round of
> >> review, while folks could also add the prefix manually to indicate the
> >> change *isn't* ready for review.
> >
> > Guido squashed the title prefix idea at the language summit (it's what I
> > originally proposed, so this label approach was the compromise).
>
> Ah, fair enough. In that case, I agree that a "Bedevere: ready for
> review" comment to request removal of the label make sense.
>
> >> > 4. Awaiting Nth review/re-review: the PR submitter believes all the
> >> > requests have been addressed in all reviews and so is waiting on the
> >> > reviewer(s) who requested changes to review again (will need to
> provide a
> >> > way to leave a message that signals to Bedevere that the submitter
> feels
> >> > ready for another review)
> >>
> >> I don't see a need for this state - going back to "Awaiting core
> >> review" should suffice.
> >
> > So the reason I made this separate is if you're in the middle of working
> > with someone on a PR and you're already reviewing it, do I really need to
> > get involved? If I'm perusing the list of open PRs, wouldn't it be better
> > for me to look for another PR that has zero reviews than pile on to
> another
> > PR that already has a core dev reviewing it?
>
> OK, that rationale makes sense. Given that, I'd suggest making the
> three review states:
>
> - awaiting review
> - awaiting core review
> - awaiting change review
>
> >> > 5. Awaiting merge: all reviews approve of the PR (we could also drop
> the
> >> > "awaiting core review" stage and go straight here even for non-core
> reviews
> >> > that are all approvals, but maybe that's leaning on non-core devs too
> much
> >> > and giving false hope to PR submitters?)
> >>
> >> I assume we'd mainly get here based on all positive core reviews, no
> >> pending negative core reviews, but a pending CI run to check
> >> everything is working as expected.
> >
> > It can also be reached even if the CI is cleared but the PR will require
> > backporting and the person who plans to do the merge + backport doesn't
> have
> > the time ATM (e.g. I did a review of a PR that applies to master and 3.6
> on
> > my lunch break, but I'm waiting until I have time to run cherry_picker at
> > home to do the merge so I don't lose track of the fact that I need to do
> the
> > backport).
>
> Aye, I can see that.
>
> FWIW, I was doing things that way for a while, but eventually realised
> it made more sense for me to make use of the "backport needed" state
> on BPO, since that's closer to our target workflow with automated
> backports. It also means I can batch up a bunch of relatively
> mechanical backports to do via a local checkout later, rather than
> having to switch back and forth between code review and backporting
> things.
>
> It definitely isn't perfect (Mariatta ended up cleaning up a few of
> the backports from the last batch I did after I'd left them pending
> for over a week), but given that the original PRs had previously been
> lingering for months, I still count it as an improvement over my
> initial approach ;)
>
> Cheers,
> Nick.
>
> --
> Nick Coghlan | ncogh...@gmail.com | Brisbane, Australia
>
_______________________________________________
core-workflow mailing list
core-workflow@python.org
https://mail.python.org/mailman/listinfo/core-workflow
This list is governed by the PSF Code of Conduct:
https://www.python.org/psf/codeofconduct
