I have some closed/merged PRs that have one of the "awaiting" on them, and I guess it's common to forget to remove tags. I believe these are unnecessary entries that clutter the view when you people filter the list.
I thought they could be automatically removed when a PR is closed/merged. They aren't useful anymore, right?
I believe that Awaiting usually is auto-removed when a PR is merged. Please posts links to examples where this did not occur. Removing when closed would be a nuisance if the closure is temporary to re-trigger CI bots.
All "awaiting ..." labels should have been removed when the PR is merged (done in #58).
On purpose the "awaiting ..." label isn't removed the PR get closed, and not merged. Sometimes we need to close and re-open a PR for various reasons (mistakenly pressed "close and comment", or needing to retrigger the CI). By leaving the labels, at least we can continue the stage it was in, if we decide to reopen the PR. But maybe we need to re-evaluate that. I simply didn't do any research on the "re-opened PR" workflow :)
Consider a scenario where a PR has been approved by a core dev, and it has "awaiting merge" label.
If we accidentally close it, and the "awaiting merge" label was removed, can the bot re-add the label if we decide to re-open? If GitHub did not wipe the "approved" state after it was re-opened, perhaps the bot will add the label accordingly anyway?
I just searched for merged PRs with any "awaiting ..." label. There really shouldn't be any, but:
We tested that "awaiting merge" label gets removed after the PR is merged. There was no test for the other "awaiting ..." labels.
We should add additional test cases that other "awaiting ..." labels are removed too.
Activity
andresdelfino commented on Jun 25, 2018
This might be useless, as I just noticed all the PRs are shown in an "Open" tab 😛
terryjreedy commented on Jun 25, 2018
I believe that Awaiting usually is auto-removed when a PR is merged. Please posts links to examples where this did not occur. Removing when closed would be a nuisance if the closure is temporary to re-trigger CI bots.
andresdelfino commented on Jun 25, 2018
Here are all my closed/merged PRs with "awaiting" labels:
python/cpython#7769
python/cpython#7741
python/cpython#7728
python/cpython#7694
python/cpython#7673
python/cpython#7608
python/cpython#7586
python/cpython#7538
python/cpython#7535
python/cpython#6821
python/cpython#6697
python/cpython#6603
(edit: I know get Terry was asking for examples where labels are removed when merging PRs)
I didn't know there were temporary closures :P
As I said in my second comment, the proposed behaviour may be useless.
Mariatta commented on Jun 25, 2018
All "awaiting ..." labels should have been removed when the PR is merged (done in #58).
On purpose the "awaiting ..." label isn't removed the PR get closed, and not merged. Sometimes we need to close and re-open a PR for various reasons (mistakenly pressed "close and comment", or needing to retrigger the CI). By leaving the labels, at least we can continue the stage it was in, if we decide to reopen the PR. But maybe we need to re-evaluate that. I simply didn't do any research on the "re-opened PR" workflow :)
Consider a scenario where a PR has been approved by a core dev, and it has "awaiting merge" label.
If we accidentally close it, and the "awaiting merge" label was removed, can the bot re-add the label if we decide to re-open? If GitHub did not wipe the "approved" state after it was re-opened, perhaps the bot will add the label accordingly anyway?
I just searched for merged PRs with any "awaiting ..." label. There really shouldn't be any, but:
merged and awaiting change review no result
merged and awaiting changes no result
merged and awaiting core review 1 result
merged and awaiting merge no result
merged and awaiting review 1 result
I will look into why those aren't removed.
Mariatta commented on Jun 25, 2018
@andresdelfino One thing you can do is to add additional test cases.
bedevere/tests/test_stage.py
Line 545 in 7bc79be
We tested that "awaiting merge" label gets removed after the PR is merged. There was no test for the other "awaiting ..." labels.
We should add additional test cases that other "awaiting ..." labels are removed too.
andresdelfino commented on Jun 25, 2018
I definitely see the advantages of the current behaviour now that you mention this.
Sure, I'll work on that :) Thanks for mentioning it!
Mariatta commented on Jun 25, 2018
I've manually removed those labels.
Improve the test cases of awaiting labels removal (#118)