Add alias for compound labels #2172
Conversation
apiraino
force-pushed
the
alias-for-compound-labels
branch
11 times, most recently
from
f33ff92 to
6483af4
Compare
apiraino
force-pushed
the
alias-for-compound-labels
branch
from
6483af4 to
fd2c1cb
Compare
| LabelDelta::Remove(label) => { | ||
| results.push(( | ||
| label, | ||
| event.issue().unwrap().remove_label(&ctx.github, &label), | ||
| )); | ||
| to_rem.push(label); | ||
| } |
There was a problem hiding this comment.
here I changed a bit the logic: instead of storing the futures I just store the labels to be removed and then execute the futures later. Functionally I think it changes nothing but the original version (introduced in b6ccaa0) created a lifetime error after my my patch.
|
I think now it's ready for review. I've tested this patch on my test repository r? triagebot |
|
|
||
| // If the input matches a valid alias, read the [relabel] config. | ||
| // if any alias matches, extract the alias config (RelabelRuleConfig) and build a new RelabelCommand. | ||
| // Discard anything after the alias. |
There was a problem hiding this comment.
I don't think we should discard anything, at least not silently.
In my opinion we should simply expand all the aliases, and if someone does @rustbot label +label +alias +other-label I would expect both label and other-label to be added + whatever alias expands to.
There was a problem hiding this comment.
When I implemented this patch I thought about allowing both labels and aliases but ultimately discarded the option. My reasoning was that it would open up to contradicting labelling cases:
[config.alias]
add-label = ["P-high"]
rem-label = ["I-prioritize"]
# should I-prioritize stay or not?
@rustbot label alias I-prioritize
I think a user should either send ONE command alias or a set of labels, not both. If a user wants to send many labels, they can configure an alias.
There was a problem hiding this comment.
We already have the issue when someone does @rustbot labels I-prioritize -I-prioritize I-prioritize, currently we adds all the labels first and then removes the other one (resulting in always no label, no matter the position).
IMO the behavior should be made consistent by using the precedence, from left to right, so in either case (your example and mine) I-prioritize would be added to the issue.
But whatever the behavior I don't this issue as being big enough compare to not allow regular labels and labels to be mixed together, which would just looked like an arbitrary limitation. I could very well see my-self wanting to do @rustbot labels +diagnostic-alias +F-my-feature (where diagnostic-alias = +T-compiler +A-diagnostics) and expecting it to work.
We could also error out if the alias and labels conflict, it's less pretty but still an option.
| // get only the first token from the command | ||
| // extract the "alias" from the RelabelCommand |
There was a problem hiding this comment.
What's the reason for only considering the first token as an alias?
I would expect aliases to work at any position.
There was a problem hiding this comment.
see previous comment
Discussion on #triagebot > Generic shorcut handler @ 💬).
WIP, currently blocked on a lifetime issue 🙄
Closes #1634