Merge pull request #632 from Kobzol/command-delegate

Improve delegate command error message and add `@bors delegate` alias

Author: Kobzol

src/bors/command/parser.rs

@@ -14,7 +14,7 @@ pub enum CommandParseError {
     MissingCommand,
     UnknownCommand(String),
     MissingArgValue { arg: String },
-    UnknownArg(String),
+    UnknownArg { arg: String, did_you_mean: String },
     DuplicateArg(String),
     ValidationError(String),
 }
@@ -270,7 +270,10 @@ fn parser_try(command: &CommandPart<'_>, parts: &[CommandPart<'_>]) -> ParseResu
     for part in parts {
         match part {
             CommandPart::Bare(key) => {
-                return Some(Err(CommandParseError::UnknownArg(key.to_string())));
+                return Some(Err(CommandParseError::UnknownArg {
+                    arg: key.to_string(),
+                    did_you_mean: "jobs=<jobs>|parent=<parent>".to_string(),
+                }));
             }
             CommandPart::KeyValue { key, value } => match (*key, *value) {
                 ("parent", "last") => parent = Some(Parent::Last),
@@ -299,7 +302,10 @@ fn parser_try(command: &CommandPart<'_>, parts: &[CommandPart<'_>]) -> ParseResu
                     jobs = raw_jobs;
                 }
                 _ => {
-                    return Some(Err(CommandParseError::UnknownArg(key.to_string())));
+                    return Some(Err(CommandParseError::UnknownArg {
+                        arg: key.to_string(),
+                        did_you_mean: "jobs=<jobs>|parent=<parent>".to_string(),
+                    }));
                 }
             },
         }
@@ -318,11 +324,24 @@ fn parser_try_cancel(command: &CommandPart<'_>, parts: &[CommandPart<'_>]) -> Pa
 }
 
 /// Parses `@bors delegate=<try|review>` or `@bors delegate+`.
-fn parser_delegate(command: &CommandPart<'_>, _parts: &[CommandPart<'_>]) -> ParseResult {
+fn parser_delegate(command: &CommandPart<'_>, parts: &[CommandPart<'_>]) -> ParseResult {
     match command {
         CommandPart::Bare("delegate+") => {
             Some(Ok(BorsCommand::SetDelegate(DelegatedPermission::Review)))
         }
+        CommandPart::Bare("delegate") => {
+            if !parts.is_empty() {
+                Some(Err(CommandParseError::UnknownArg {
+                    arg: match parts[0] {
+                        CommandPart::Bare(arg) => arg.to_owned(),
+                        CommandPart::KeyValue { key, .. } => key.to_owned(),
+                    },
+                    did_you_mean: "delegate=<try|review>".to_string(),
+                }))
+            } else {
+                Some(Ok(BorsCommand::SetDelegate(DelegatedPermission::Review)))
+            }
+        }
         CommandPart::KeyValue {
             key: "delegate",
             value,
@@ -1163,15 +1182,31 @@ for the crater",
     #[test]
     fn parse_try_unknown_arg() {
         let cmds = parse_commands("@bors try a");
-        assert_eq!(cmds.len(), 1);
-        assert_eq!(cmds[0], Err(CommandParseError::UnknownArg("a".to_string())));
+        insta::assert_debug_snapshot!(cmds, @r#"
+        [
+            Err(
+                UnknownArg {
+                    arg: "a",
+                    did_you_mean: "jobs=<jobs>|parent=<parent>",
+                },
+            ),
+        ]
+        "#);
     }
 
     #[test]
     fn parse_try_unknown_kv_arg() {
         let cmds = parse_commands("@bors try a=b");
-        assert_eq!(cmds.len(), 1);
-        assert_eq!(cmds[0], Err(CommandParseError::UnknownArg("a".to_string())));
+        insta::assert_debug_snapshot!(cmds, @r#"
+        [
+            Err(
+                UnknownArg {
+                    arg: "a",
+                    did_you_mean: "jobs=<jobs>|parent=<parent>",
+                },
+            ),
+        ]
+        "#);
     }
 
     #[test]
@@ -1228,7 +1263,7 @@ for the crater",
     }
 
     #[test]
-    fn parse_delegate_author() {
+    fn parse_delegate_plus_author() {
         let cmds = parse_commands("@bors delegate+");
         assert_eq!(cmds.len(), 1);
         assert!(matches!(
@@ -1237,6 +1272,31 @@ for the crater",
         ));
     }
 
+    #[test]
+    fn parse_delegate_author() {
+        let cmds = parse_commands("@bors delegate");
+        assert_eq!(cmds.len(), 1);
+        assert!(matches!(
+            cmds[0],
+            Ok(BorsCommand::SetDelegate(DelegatedPermission::Review))
+        ));
+    }
+
+    #[test]
+    fn parse_delegate_unknown_arg() {
+        let cmds = parse_commands("@bors delegate try");
+        insta::assert_debug_snapshot!(cmds, @r#"
+        [
+            Err(
+                UnknownArg {
+                    arg: "try",
+                    did_you_mean: "delegate=<try|review>",
+                },
+            ),
+        ]
+        "#);
+    }
+
     #[test]
     fn parse_delegate_review_author() {
         let cmds = parse_commands("@bors delegate=review");

src/bors/handlers/help.rs

@@ -43,7 +43,7 @@ mod tests {
             - `delegate=<try|review>`: Delegate permissions for running try builds or approving to the PR author
                 - `try` allows the PR author to start try builds.
                 - `review` allows the PR author to both start try builds and approve the PR.
-            - `delegate+`: Delegate approval permissions to the PR author
+            - `delegate` | `delegate+`: Delegate approval permissions to the PR author
                 - Shortcut for `delegate=review`
             - `delegate-`: Remove any previously granted permission delegation
             - `try [parent=<parent>] [job|jobs=<jobs>]`: Start a try build.

src/bors/handlers/mod.rs

@@ -549,8 +549,11 @@ async fn handle_comment(
                     CommandParseError::MissingArgValue { arg } => {
                         format!(r#"Unknown value for argument "{arg}"."#)
                     }
-                    CommandParseError::UnknownArg(arg) => {
-                        format!(r#"Unknown argument "{arg}"."#)
+                    CommandParseError::UnknownArg { arg, did_you_mean } => {
+                        format!(
+                            r#"Unknown argument "{arg}". Did you mean to use `{} {did_you_mean}`?"#,
+                            ctx.parser.prefix()
+                        )
                     }
                     CommandParseError::DuplicateArg(arg) => {
                         format!(r#"Argument "{arg}" found multiple times."#)

src/bors/handlers/review.rs

@@ -882,6 +882,19 @@ approved = { modifications = ["+foo", "+baz"], unless = ["label1", "label2"] }
             .await;
     }
 
+    #[sqlx::test]
+    async fn delegate_error_message(pool: sqlx::PgPool) {
+        BorsBuilder::new(pool)
+            .github(GitHub::unauthorized_pr_author())
+            .run_test(async |ctx: &mut BorsTester| {
+                ctx.post_comment(review_comment("@bors delegate try"))
+                    .await?;
+                insta::assert_snapshot!(ctx.get_next_comment_text(()).await?, @r#"Unknown argument "try". Did you mean to use `@bors delegate=<try|review>`? Run `@bors help` to see available commands."#);
+                Ok(())
+            })
+            .await;
+    }
+
     #[sqlx::test]
     async fn delegatee_can_try(pool: sqlx::PgPool) {
         let gh = BorsBuilder::new(pool)

src/bors/mod.rs

@@ -97,7 +97,7 @@ You can use the following commands:
 - `delegate=<try|review>`: Delegate permissions for running try builds or approving to the PR author
     - `try` allows the PR author to start try builds.
     - `review` allows the PR author to both start try builds and approve the PR.
-- `delegate+`: Delegate approval permissions to the PR author
+- `delegate` | `delegate+`: Delegate approval permissions to the PR author
     - Shortcut for `delegate=review`
 - `delegate-`: Remove any previously granted permission delegation
 - `try [parent=<parent>] [job|jobs=<jobs>]`: Start a try build.