dlang-bot icon indicating copy to clipboard operation
dlang-bot copied to clipboard

[WIP] Fix #191 - remove common attributes when not in PR title

Open berni44 opened this issue 4 years ago • 10 comments

I did not find a way to compare the old title with the new one, so I choose a simple approach to remove all labels, that are not listed in the title, whenever the title is changed.

Drawback of this solution is, when someone with write access adds one of the labels manually and changes the title later, the label will be removed - that means, people with write access should also use the title to add and remove WIP and trivial. (But people with write access can just add the label again, while people without write access can't do anything to remove the wrong label.)

berni44 avatar Feb 21 '21 09:02 berni44

Thanks for your pull request and interest in making D better, @berni44! We are looking forward to reviewing it, and you should be hearing from a maintainer soon. Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

dlang-bot avatar Feb 21 '21 09:02 dlang-bot

I'm not familiar enough with the bot to understand, what's going wrong here. Maybe, someone can help me?

berni44 avatar Feb 21 '21 09:02 berni44

From what I can tell:

Request for unexpected URL received: '/github/repos/dlang/phobos/issues/5519/labels' Program exited with code -6 Error: Process completed with exit code 2.

https://github.com/dlang/dlang-bot/blob/52d814cdf8750d90e5bacb71e174ff32e8261ed1/test/utils.d#L112

https://github.com/dlang/dlang-bot/blob/52d814cdf8750d90e5bacb71e174ff32e8261ed1/test/utils.d#L221-L231

https://github.com/dlang/dlang-bot/blob/52d814cdf8750d90e5bacb71e174ff32e8261ed1/test/labels.d#L18-L26

You need to update one or more unit tests.

Even if the CI was not failing, I'd say that you still need to add a test coverage for the new behavior.

Edit: See where the PR number 5519 occurs: https://github.com/dlang/dlang-bot/search?q=5519&type=

PetarKirov avatar Feb 22 '21 08:02 PetarKirov

You need to update one or more unit tests.

Meanwhile I updated some. Yet it's still failing. Will check this the other day...

Even if the CI was not failing, I'd say that you still need to add a test coverage for the new behavior.

Yes, I'll do. But first I'd like to get the existing unittests running...

berni44 avatar Feb 22 '21 14:02 berni44

This is now a completely different approach: Instead of automatically removing labels, that are not present in the title, the bot removes labels, when they are preceded by a minus in the title, like [-wip]. This has the advantage, that no unittests need to be changed.

Unfortunately I did not manage to write a unittest, that tests the new features. My knowledge about all this is too limited to guess, what I need to do. Maybe it's possible to hijack the "label-via-title" test and remove "trivial" there. I don't know how to do this, because it's not another opening of the same test, but this time a edited event. I don't know how the corresponding json file should look like. Alternatively one could use a separate test. In that case, a label must be set somehow before the test is run. Again I don't know how to do this.

So all in all: I have to give up at this stage. :-(

berni44 avatar Feb 22 '21 17:02 berni44

What problem is this attempting to solve?

CyberShadow avatar May 27 '21 01:05 CyberShadow

What problem is this attempting to solve?

People, who have no write access can add WIP label but not remove them, which renders the ability of adding them quite useless. Once added, people do not look at the PR anymore and so no one cares about the PR and you can't remove that label. All in all, the "feature" to add the WIP label turns out to be a trap (without this).

berni44 avatar Jun 04 '21 19:06 berni44

Why would you want to use WIP labels instead of draft pull requests?

CyberShadow avatar Jun 04 '21 20:06 CyberShadow

Why would you want to use WIP labels instead of draft pull requests?

Then: Why does the WIP label exist and why can users without write access set it?

berni44 avatar Jun 11 '21 12:06 berni44

Why does the WIP label exist

It predates the GitHub draft pull request feature by a few years.

and why can users without write access set it?

Before the GitHub feature was added, many people used the convention of prefixing the pull request title with "[WIP] " or such. @dlang-bot picked up on this convention and added the label to allow easily filtering such PRs, I guess.

CyberShadow avatar Jun 11 '21 12:06 CyberShadow