Add support for @bot claim @user to assign to another user - #2456
Conversation
So that when this command works to assign another user the difference is clear in tests.
…n-user So that when we add support for `@bot claim @user` we don't accidentally introduce errors when `@bot claim` is followed by a non-username.
| impl AssignCommand { | ||
| pub fn parse<'a>(input: &mut Tokenizer<'a>) -> Result<Option<Self>, Error<'a>> { | ||
| pub fn parse<'a>( | ||
| bots: &Vec<&'a str>, |
There was a problem hiding this comment.
Why do you filter out bots? I don't think we do that elsewhere, or do we?
There was a problem hiding this comment.
I'm not aware either of any other place where we stop parsing at a bot handle. Generally, when we want to stop parsing we look for a punctuation like ..
There was a problem hiding this comment.
I wanted to ensure commands like @rustbot claim @rustbot label +bug continued to be parsed as two different commands rather than assigning the bot itself
There was a problem hiding this comment.
That's not consistent with what we do for other commands here, I think. bors does stop at @ to let other bots handle the rest of the line, but triagebot AFAIK doesn't, and to add insult to injury, it actually requires @ to be present before usernames for the assign (and now claim) commands, which is IMO unnecessary.
Please remove the bot handling, we don't do it elsewhere in triagebot.
There was a problem hiding this comment.
Okay, removed - it turns out that the bot still handles the labeling after the assignment, see the changes in test_follow_commands()
…mand So that when we add support for `@bot claim @user` we can see the behavior change.
f8fded9 to
277cbca
Compare
There was a problem hiding this comment.
Looks fine to me, I'll leave @Urgau to take a look if you want.
3 participants
Fixes #2451