search

pypa / pip

The Python package installer
https://pip.pypa.io/
MIT License
9.51k stars 3.02k forks source link

Bots to manage PR state in labels #7306

Open pradyunsg opened 4 years ago

pradyunsg commented 4 years ago

If PRs automatically get a label describing what state they're in, IMO it'll make it easier to handle PR reviews. To copy from CPython, I think we'd benefit from these labels:

I think the approximate flow would be:

We'd still be able to manually apply these labels and if the above is the behavior of the bots, I don't think they'd fight us. Do others think this would be a good idea?

pfmoore commented 4 years ago

LGTM.

new PR: apply S: awaiting review once CI passes (or on open?)

When CI passes. If someone wants a review before they have a clean CI, they can ask for one (with an explanation if needed) but I think it's good that we take the stance that CI passing is the basic criterion we'd expect.

chrahunt commented 4 years ago

I would apply S: awaiting review once required CI passes - we don't want experiments with CI checks to mess up our workflow. I think this would be an improvement over the red X that currently shows on the PR overview page since an S: awaiting review with red X would indicate that some secondary check has failed, not the main test suite/lints and it's probably worth taking a look.

I don't know if I would find S: awaiting merge useful as-is - if I approve something and don't merge it immediately it is because:

  1. the change is nontrivial and another maintainer may have an opinion in this area but I don't think time-wise they've had enough opportunity to review it (maybe a 24 hour window depending on time zones and whether people are active on other issues)
  2. another maintainer has commented (possibly as "comment" not just "changes requested") and there hasn't been that much time since the latest change that it's reasonable to assume they've reviewed it
  3. the PR was submitted by a maintainer and they can merge at their discretion i.e. nothing immediately depends on the changes in that PR

in the first two cases keeping S: awaiting review would indicate that someone should take a look and if that someone thinks it's OK to merge then they can do it at that point.

pradyunsg commented 4 years ago

My goal with awaiting merge is to avoid PRs from lingering. Any suggestions on how we could do that? Maybe some combination of delays or something else entirely?

chrahunt commented 4 years ago

Maybe I'm misunderstanding. If the transition to S: awaiting merge requires a maintainer action (the last person with "changes requested" approves) and it means that it can be merged, wouldn't that maintainer merge it?

Also what phase are we concerned about PRs lingering in? Awaiting review or response or approved but not merged?

pradyunsg commented 4 years ago

Also what phase are we concerned about PRs lingering in?

Approved and not merged.