rust-lang / triagebot

Automation/tooling for Rust spaces
https://triage.rust-lang.org
Apache License 2.0
172 stars 73 forks source link

Comment on issues when `@rustbot label` is given an invalid label #1610

Closed jyn514 closed 2 years ago

jyn514 commented 2 years ago

Previously, the label was just silently ignored, along with all other labels in the same command. This tells the user what went wrong, and also adds all valid labels.

I'm not sure how to test this :(

Mark-Simulacrum commented 2 years ago

A couple notes:

I'm also a little uncertain about the approach. I think historically the choice to silently eat bad labels has been semi-intentional, as it means that we can avoid lots of comments if you edit a comment trying to fix any typos you made. This definitely prioritizes "advanced" users over newcomers though; which might well be the wrong tradeoff.

I'm not sure what a good way to thread the needle on that tradeoff is. It seems like either way we're not really delivering a great experience.

Regardless, my inclination is to make label adding atomic, like we did before. I think it's much less confusing what to do when we've failed to add in general, and prevents awkwardness around label-based triggering on "half" of the labels you added (since that triggering can have exclusions).

jyn514 commented 2 years ago

Regardless, my inclination is to make label adding atomic, like we did before.

👍 This makes sense to me and will simplify moving the changes out of github.rs. I'll try and get to it this week.

jyn514 commented 2 years ago

Ah, I just realized that add_labels is used in many places besides autolabel - the tradeoffs are making more sense to me now. I changed this to return an error on unknown labels that only autolabel checks for, and all other commands just bubble up to the top-level as a log. I also ensured that no labels are added if unknown labels are present. Note that this is not quite the same as "atomic" because autolabel calls add_labels(); remove_labels(); right after each other, but it matches the previous behavior.