Non-core review downgrades core dev PRs

[pganssle]

This PR from @vstinner was initially labeled "awaiting merge", which I suppose is the default behavior when a core dev makes a PR, even before review.

I (not a core dev) reviewed the code and approved the changes, which triggered bedevere to remove "awaiting merge" and add "awaiting core review".

I'm guessing this is because my review triggered the logic that normally transitions from "awaiting review" to "awaiting core review", and this probably checked to see if there were any reviews from core devs (there were not) and assumed that core review was necessary.

I would think the correct behavior for an accepted review would be to leave "awaiting merge" in place. For a rejected review, adding "awaiting core review" might be the right thing to do.

[pganssle]

I believe the problem is here.

[Mariatta]

I'm thinking it's ok for a core dev's PR to be "awaiting core review" after a review by a non core-dev.
Or perhaps only add "awaiting core review" if the review is "Changes requested"?

[pganssle]

@Mariatta Yes, I think the second one is the best option. I think the main thing is that an additional approval from a non-core dev downgrades you from "awaiting merge" to "awaiting core review". I see three self-consistent outcomes:

1. Core devs start out in the "awaiting review" stage like everyone else (possibly they can approve their own PR to get to "awaiting merge"), non-core review tags as "awaiting core review"
2. Non-core review should only downgrade from "awaiting merge" if there are changes requested and no core dev has approved the PR.
3. Non-core review should have no effect on "awaiting merge" one way or the other.

I would be in favor of options 1 or 2. Option 2 is probably the best option in that it has minimal effect on workflow and rationalizes the effect of non-core review. I personally like having my code reviewed, so if I were a core dev I'd probably want option 1, but I understand that if a lot of existing core devs don't want to go through a ritual of clicking "approve" on their own PRs before clicking "merge" it makes sense to leave that part as-is.

[terryjreedy]

As a Python core dev, I would not mind 1. I generally like having reviews, and I often do not consider my PRs ready to merge at least until I have slept on it and done a final pre-merge review.

Github does not allow author reviews. So option 1 does not add a ritual of doing so. A PR author can comment anyway. Core-devs do not need to formally 'approve' a PR before merging it.

An issue being marked 'ready to merge' without an approve review says that the author is a core developer. But one can determine that by hovering the mouse over the image/icon.

[Mariatta]

IMO Option 1) will bring the most disruptive change to our existing workflow. While it is true for core devs the "awaiting review" label is mainly for informational purpose, and does not block merging, our other bot miss-islington depend on the "awaiting merge" label for automerge. So if core devs PRs will always start with "awaiting review" now, it means most of them can't be easily automatically merged.

I would choose to go with option 2)

[terryjreedy]

OK. I have never used auomerge, so I did not consider it. 2 is with me too.

[lysnikolaou]

I think, that since #157 this is no longer an issue, so it can be closed.

[brettcannon]

@lysnikolaou there might still be an issue of accidentally lowering the status, so I'm not quite ready to close this issue until we have a test to make sure you can't accidentally change the state.

[potomak]

I'm not quite ready to close this issue until we have a test to make sure you can't accidentally change the state.

@brettcannon do you think we can close this issue now that there's a test specifically for this case?

[brettcannon]

@potomak yes we can!