I fundamentally support the idea of distinguishing the two cases better.

I'm always hesitant with request changes because somebody has to dismiss the review afterwards, and GitHub makes quite a song and dance out of that, so that it feels awkward to dismiss someone's else review,
But technically the process is right. We should encourage that any reviewer can and should dismiss if they see that the comments are properly addressed.

Note, I couldn't add a rule on this workflow passing yet, probably because it's not had any runs in this repo. So we can't really preview it (at least with this workflow).

So we can't really preview it (at least with this workflow).

Have I managed a catch 22 where we can't see how it fully works before discussing b/c that requires a merge but it'd be nice to see how it fully works before merging? 😶

I could set it on other checks, but that would likely affect other PRs. However, I don't think it would look too different from how it does now with the "Merging is blocked" error due to not having any approving reviews.

However, I don't think it would look too different from how it does now with the "Merging is blocked" error due to not having any approving reviews.

Yeah, in all seriousness I figure it'll work like the required label check on any sphinx gallery PR -> which👀 how they label the required ones

This seems reasonable.

@ksunden has a pass and a fail state now

Hmm, I thought we had merged this already. It's not really waiting for another PR, right? That was just for testing?

It's not really waiting for another PR, right? That was just for testing?

Yup!

Though now it doesn't seem to be working 🤦‍♀️

It looks green to me?

This is what I see? Maybe it's cause I'm on the app?

Here I see it passing (the 1 failing is codecov):

But confusingly, if I go to the Checks tab, then there are four copies of this check, and two of them failed. But the three old ones are from Feb 8, passing is from 2 days ago.

ok, so looks like it's working but there's just some lag sometimes

I'm not sold on the utility of this, and it just seems to needlessly complicate the status of PRs. We already have "block" and "Draft" as tools to indicate changes need to be made, or we can even go so far as to close PRs. We can also communicate in the comments. With this change we have to further remember a list of tags that will opaquely block merging and remember which ones we need to remove to un-block. That doesn't seem that helpful to our workflows.

With this change we have to further remember a list of tags that will opaquely block merging and remember which ones we need to remove to un-block

It's the two tags that already informally mean "don't merge" without doing the further action described in the tag . Basically all I'm doing here is programmatically enforcing an unwritten convention we already have.

I've generally used those tags in the past to mean that something needs input from other people, and/or discussion at the weekly meeting. I'm not sure I've ever used them to mean "don't merge this".

I've generally used those tags in the past to mean that something needs input from other people, and/or discussion at the weekly meeting. I'm not sure I've ever used them to mean "don't merge this".

I use it to mean "needs input/discussion" because this does not have consensus and therefore it should not be merged. Per our definition

I only ever use it to mean "hey, don't merge this" (and has always been how I interpret it) and generally just tag people if I think something needs more general input.

I only resort to using "request changes" as a block when I feel the label is getting ignored (which per this conversation is apparently b/c we have very mismatched expectations), b/c as I said up top, I fundamentally think we should be using different mechanisms to communicate:

1. needs change but will be merged after change is implemented
2. needs meta discussion, may not be merged

One of my hopes here is that making the mappings clearer will make it easier for folks to dismiss changes or remove labels when the issue is addressed.

We've already discussed this on the call and decided to try it out, and see if it's effective.

This is now required on PRs. I couldn't find it as the workflow name, but it turns out it's listed by job name.