Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement the shortcut handler #1381

Merged
merged 6 commits into from
Jul 18, 2021
Merged

Conversation

Llandy3d
Copy link
Contributor

@Llandy3d Llandy3d commented Jun 7, 2021

#1339

This implements the shorcut handler and already works with two shorcuts:

  • ready -> S-waiting-on-review
  • author -> S-waiting-on-author

It correctly remove the unnecessary label while keeping previous ones. This functionality is accompanied by unit tests.

I do wonder if the author command should be allowed to only team members or it's ok as it is allowed now.

A follow up to this that require more analysis, would be to ping assigned reviewers.

@Llandy3d
Copy link
Contributor Author

Llandy3d commented Jun 7, 2021

Now it will also ping the appropriate people by leaving a comment:

  • ready -> Assignees
  • author -> User that opened the pull request

@nikomatsakis nikomatsakis self-assigned this Jun 8, 2021
impl ShortcutCommand {
pub fn parse<'a>(input: &mut Tokenizer<'a>) -> Result<Option<Self>, Error<'a>> {
let mut toks = input.clone();
if let Some(Token::Word("ready")) = toks.peek_token()? {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we can remove some of this duplicated code. Maybe something like

let shortcuts = [
    ("ready", ShortcutCommand::Ready),
    ("author", ShortcutCommand::Author),
];

for (word, command) in shortcuts {
    if let Some(Token::Word(word)) = toks.peek_token()? { ... }
}

Ok(None)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By using an Hashmap we can avoid looping at all, while having clear the available options

@nikomatsakis
Copy link
Contributor

@Llandy3d ok, I'm ready to merge, but it needs a rebase

@Llandy3d
Copy link
Contributor Author

@nikomatsakis rebased & resolved conflicts

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question @Llandy3d -- do we actually check the config at any point? In other words, shortcuts are just always enabled, right? Are the "label editing" commands always enabled? Maybe that checking already happens in set_labels?

@Llandy3d
Copy link
Contributor Author

Question @Llandy3d -- do we actually check the config at any point? In other words, shortcuts are just always enabled, right? Are the "label editing" commands always enabled? Maybe that checking already happens in set_labels?

Features are not always enabled by default, the check happens in handlers.rs, it seems to happen in the command_handlers macro.

To maintain the current behaviour it's mandatory to configure the shortcuts, if you don't and try to use them you will be greeted by the standard error message:
image

So the [shortcut] config in triagebot.toml is required

@nikomatsakis
Copy link
Contributor

Got it! Going to merge now, then @Llandy3d can you update the wiki?

@Llandy3d
Copy link
Contributor Author

@nikomatsakis sure for the wiki, somehow when running tests from the root of the repo it wasn't running parser's tests, fixed a case where it would return a ParseError instead of Ok(None)

@Llandy3d
Copy link
Contributor Author

The corresponding wiki page can be found here

@nikomatsakis nikomatsakis merged commit ef61cb1 into rust-lang:master Jul 18, 2021
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jul 19, 2021
…omatsakis

triagebot shortcut config

Enable the new triagebot shortcuts as per [rust-lang#1381/triagebot](rust-lang/triagebot#1381)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants