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

[berni44]

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.)

[dlang-bot]

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.

[berni44]

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

[PetarKirov]

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.

dlang-bot/test/utils.d

Line 112 in 52d814c

logError("Request for unexpected URL received: '%s'", req.requestURL);

dlang-bot/test/utils.d

Lines 221 to 231 in 52d814c

	void setAPIExpectations(Args...)(Args args)
	{
	import std.functional : toDelegate;
	import std.traits : Parameters;
	synchronized {
	apiExpectations.length = 0;
	foreach (i, arg; args)
	{
	static if (is(Args[i] : string))
	{
	apiExpectations ~= APIExpectation(arg);

dlang-bot/test/labels.d

Lines 18 to 26 in 52d814c

	unittest
	{
	setAPIExpectations(
	"/github/repos/dlang/phobos/commits/d2c7d3761b73405ee39da3fd7fe5030dee35a39e/status",
	"/github/repos/dlang/phobos/pulls/4921/commits",
	"/github/repos/dlang/phobos/issues/4921/labels", (ref Json j) {
	j[0]["name"] = "auto-merge";
	},
	);

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=

[berni44]

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]

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. :-(

[CyberShadow]

What problem is this attempting to solve?

[berni44]

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).

[CyberShadow]

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

[berni44]

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?

[CyberShadow]

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.