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", and have it advance only for a positive review with no negative reviews. > 2. Awaiting core review: a non-core dev has reviewed the PR, but there still > needs to be that initial core review (still not sure if this is worth the > distinction or how complex it will make the code, but it may encourage people > to help in the reviewing process instead of automatically diving into coding > if their review leads to a visible stage change) 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. > 3. Awaiting changes: a review has been done, but changes were requested > (probably wouldn't reach this stage for non-core reviews; a bit tough to > block on a non-core review as their asks may not be reasonable and so > shouldn't automatically signal that what someone that they must do what is > requested of them in that instance) +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. (GitLab has specific handling for "WIP:" and "[WIP]" to prevent merges, and it's really handy as a communications tool when you want feedback on a draft change, but also want to make it completely clear that the change isn't ready to be merged yet) > 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. > 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. At some point in the future, we could potentially even go to an OpenStack style flow where Bedevere actually *merges* patches in this state if their CI run succeeds, so it wouldn't be reasonable to get here based solely on non-core reviews. 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