Merge pull request #708 from kellda/multicmd

Allow multiple commands in a single comment

Author: Mark-Simulacrum

parser/src/command.rs

@@ -23,7 +23,6 @@ pub enum Command<'a> {
     Prioritize(Result<prioritize::PrioritizeCommand, Error<'a>>),
     Second(Result<second::SecondCommand, Error<'a>>),
     Glacier(Result<glacier::GlacierCommand, Error<'a>>),
-    None,
 }
 
 #[derive(Debug)]
@@ -64,12 +63,7 @@ impl<'a> Input<'a> {
         }
     }
 
-    pub fn parse_command(&mut self) -> Command<'a> {
-        let start = match find_commmand_start(&self.all[self.parsed..], self.bot) {
-            Some(pos) => pos,
-            None => return Command::None,
-        };
-        self.parsed += start;
+    fn parse_command(&mut self) -> Option<Command<'a>> {
         let mut tok = Tokenizer::new(&self.all[self.parsed..]);
         assert_eq!(
             tok.next_token().unwrap(),
@@ -131,18 +125,31 @@ impl<'a> Input<'a> {
             .is_some()
         {
             log::info!("command overlaps code; code: {:?}", self.code);
-            return Command::None;
+            return None;
         }
 
-        match success.pop() {
-            Some((mut tok, c)) => {
-                // if we errored out while parsing the command do not move the input forwards
-                if c.is_ok() {
-                    self.parsed += tok.position();
-                }
-                c
+        let (mut tok, c) = success.pop()?;
+        // if we errored out while parsing the command do not move the input forwards
+        self.parsed += if c.is_ok() {
+            tok.position()
+        } else {
+            self.bot.len() + 1
+        };
+        Some(c)
+    }
+}
+
+impl<'a> Iterator for Input<'a> {
+    type Item = Command<'a>;
+
+    fn next(&mut self) -> Option<Command<'a>> {
+        loop {
+            let start = find_commmand_start(&self.all[self.parsed..], self.bot)?;
+            self.parsed += start;
+            if let Some(command) = self.parse_command() {
+                return Some(command);
             }
-            None => Command::None,
+            self.parsed += self.bot.len() + 1;
         }
     }
 }
@@ -157,35 +164,27 @@ impl<'a> Command<'a> {
             Command::Prioritize(r) => r.is_ok(),
             Command::Second(r) => r.is_ok(),
             Command::Glacier(r) => r.is_ok(),
-            Command::None => true,
         }
     }
 
     pub fn is_err(&self) -> bool {
         !self.is_ok()
     }
-
-    pub fn is_none(&self) -> bool {
-        match self {
-            Command::None => true,
-            _ => false,
-        }
-    }
 }
 
 #[test]
 fn errors_outside_command_are_fine() {
     let input =
         "haha\" unterminated quotes @bot modify labels: +bug. Terminating after the command";
     let mut input = Input::new(input, "bot");
-    assert!(input.parse_command().is_ok());
+    assert!(input.next().unwrap().is_ok());
 }
 
 #[test]
 fn code_1() {
     let input = "`@bot modify labels: +bug.`";
     let mut input = Input::new(input, "bot");
-    assert!(input.parse_command().is_none());
+    assert!(input.next().is_none());
 }
 
 #[test]
@@ -194,7 +193,7 @@ fn code_2() {
     @bot modify labels: +bug.
     ```";
     let mut input = Input::new(input, "bot");
-    assert!(input.parse_command().is_none());
+    assert!(input.next().is_none());
 }
 
 #[test]
@@ -203,7 +202,7 @@ fn edit_1() {
     let mut input_old = Input::new(input_old, "bot");
     let input_new = "Adding labels: @bot modify labels: +bug. some other text";
     let mut input_new = Input::new(input_new, "bot");
-    assert_eq!(input_old.parse_command(), input_new.parse_command());
+    assert_eq!(input_old.next(), input_new.next());
 }
 
 #[test]
@@ -212,23 +211,22 @@ fn edit_2() {
     let mut input_old = Input::new(input_old, "bot");
     let input_new = "@bot modify labels: +bug.";
     let mut input_new = Input::new(input_new, "bot");
-    assert_ne!(input_old.parse_command(), input_new.parse_command());
+    assert_ne!(input_old.next(), input_new.next());
 }
 
 #[test]
 fn move_input_along() {
     let input = "@bot modify labels: +bug. Afterwards, delete the world.";
     let mut input = Input::new(input, "bot");
-    let parsed = input.parse_command();
-    assert!(parsed.is_ok());
+    assert!(input.next().unwrap().is_ok());
     assert_eq!(&input.all[input.parsed..], " Afterwards, delete the world.");
 }
 
 #[test]
 fn move_input_along_1() {
     let input = "@bot modify labels\": +bug. Afterwards, delete the world.";
     let mut input = Input::new(input, "bot");
-    assert!(input.parse_command().is_err());
+    assert!(input.next().unwrap().is_err());
     // don't move input along if parsing the command fails
-    assert_eq!(input.parsed, 0);
+    assert_eq!(&input.all[..input.parsed], "@bot");
 }

src/handlers.rs

@@ -35,16 +35,16 @@ mod prioritize;
 mod relabel;
 mod rustc_commits;
 
-// TODO: Return multiple handler errors ?
-pub async fn handle(ctx: &Context, event: &Event) -> Result<(), HandlerError> {
+pub async fn handle(ctx: &Context, event: &Event) -> Vec<HandlerError> {
     let config = config::get(&ctx.github, event.repo_name()).await;
+    let mut errors = Vec::new();
 
     if let (Ok(config), Event::Issue(event)) = (config.as_ref(), event) {
-        handle_issue(ctx, event, config).await?;
+        handle_issue(ctx, event, config, &mut errors).await;
     }
 
     if let Some(body) = event.comment_body() {
-        handle_command(ctx, event, &config, body).await?;
+        handle_command(ctx, event, &config, body, &mut errors).await;
     }
 
     if let Err(e) = notification::handle(ctx, event).await {
@@ -63,28 +63,34 @@ pub async fn handle(ctx: &Context, event: &Event) -> Result<(), HandlerError> {
         );
     }
 
-    Ok(())
+    errors
 }
 
 macro_rules! issue_handlers {
     ($($name:ident,)*) => {
-        async fn handle_issue(ctx: &Context, event: &IssuesEvent, config: &Arc<Config>) -> Result<(), HandlerError> {
+        async fn handle_issue(
+            ctx: &Context,
+            event: &IssuesEvent,
+            config: &Arc<Config>,
+            errors: &mut Vec<HandlerError>,
+        ) {
             $(
-            if let Some(input) = $name::parse_input(
-                ctx, event, config.$name.as_ref(),
-            ).map_err(HandlerError::Message)? {
-                if let Some(config) = &config.$name {
-                    $name::handle_input(ctx, config, event, input).await.map_err(HandlerError::Other)?;
-                } else {
-                    return Err(HandlerError::Message(format!(
-                        "The feature `{}` is not enabled in this repository.\n\
-                        To enable it add its section in the `triagebot.toml` \
-                        in the root of the repository.",
-                        stringify!($name)
-                    )));
+            match $name::parse_input(ctx, event, config.$name.as_ref()) {
+                Err(err) => errors.push(HandlerError::Message(err)),
+                Ok(Some(input)) => {
+                    if let Some(config) = &config.$name {
+                        $name::handle_input(ctx, config, event, input).await.unwrap_or_else(|err| errors.push(HandlerError::Other(err)));
+                    } else {
+                        errors.push(HandlerError::Message(format!(
+                            "The feature `{}` is not enabled in this repository.\n\
+                            To enable it add its section in the `triagebot.toml` \
+                            in the root of the repository.",
+                            stringify!($name)
+                        )));
+                    }
                 }
+                Ok(None) => {}
             })*
-            Ok(())
         }
     }
 }
@@ -101,72 +107,73 @@ issue_handlers! {
 
 macro_rules! command_handlers {
     ($($name:ident: $enum:ident,)*) => {
-        async fn handle_command(ctx: &Context, event: &Event, config: &Result<Arc<Config>, ConfigurationError>, body: &str) -> Result<(), HandlerError> {
+        async fn handle_command(
+            ctx: &Context,
+            event: &Event,
+            config: &Result<Arc<Config>, ConfigurationError>,
+            body: &str,
+            errors: &mut Vec<HandlerError>,
+        ) {
             if let Event::Issue(e) = event {
                 if !matches!(e.action, IssuesAction::Opened | IssuesAction::Edited) {
                     // no change in issue's body for these events, so skip
                     log::debug!("skipping event, issue was {:?}", e.action);
-                    return Ok(());
+                    return;
                 }
             }
 
-            // TODO: parse multiple commands and diff them
-            let mut input = Input::new(&body, &ctx.username);
-            let command = input.parse_command();
-
-            if let Some(previous) = event.comment_from() {
-                let mut prev_input = Input::new(&previous, &ctx.username);
-                let prev_command = prev_input.parse_command();
-                if command == prev_command {
-                    log::info!("skipping unmodified command: {:?} -> {:?}", prev_command, command);
-                    return Ok(());
-                } else {
-                    log::debug!("executing modified command: {:?} -> {:?}", prev_command, command);
-                }
-            }
+            let input = Input::new(&body, &ctx.username);
+            let commands = if let Some(previous) = event.comment_from() {
+                let prev_commands = Input::new(&previous, &ctx.username).collect::<Vec<_>>();
+                input.filter(|cmd| !prev_commands.contains(cmd)).collect::<Vec<_>>()
+            } else {
+                input.collect()
+            };
 
-            if command == Command::None {
-                return Ok(());
+            if commands.is_empty() {
+                return;
             }
 
             let config = match config {
                 Ok(config) => config,
                 Err(e @ ConfigurationError::Missing) => {
-                    return Err(HandlerError::Message(e.to_string()));
+                    return errors.push(HandlerError::Message(e.to_string()));
                 }
                 Err(e @ ConfigurationError::Toml(_)) => {
-                    return Err(HandlerError::Message(e.to_string()));
+                    return errors.push(HandlerError::Message(e.to_string()));
                 }
                 Err(e @ ConfigurationError::Http(_)) => {
-                    return Err(HandlerError::Other(e.clone().into()));
+                    return errors.push(HandlerError::Other(e.clone().into()));
                 }
             };
 
-            match command {
-                $(
-                Command::$enum(Ok(command)) => {
-                    if let Some(config) = &config.$name {
-                        $name::handle_command(ctx, config, event, command).await.map_err(HandlerError::Other)?;
-                    } else {
-                        return Err(HandlerError::Message(format!(
-                            "The feature `{}` is not enabled in this repository.\n\
-                            To enable it add its section in the `triagebot.toml` \
-                            in the root of the repository.",
-                            stringify!($name)
-                        )));
+            for command in commands {
+                match command {
+                    $(
+                    Command::$enum(Ok(command)) => {
+                        if let Some(config) = &config.$name {
+                            $name::handle_command(ctx, config, event, command)
+                                .await
+                                .unwrap_or_else(|err| errors.push(HandlerError::Other(err)));
+                        } else {
+                            errors.push(HandlerError::Message(format!(
+                                "The feature `{}` is not enabled in this repository.\n\
+                                To enable it add its section in the `triagebot.toml` \
+                                in the root of the repository.",
+                                stringify!($name)
+                            )));
+                        }
                     }
+                    Command::$enum(Err(err)) => {
+                        errors.push(HandlerError::Message(format!(
+                            "Parsing {} command in [comment]({}) failed: {}",
+                            stringify!($name),
+                            event.html_url().expect("has html url"),
+                            err
+                        )));
+                    })*
                 }
-                Command::$enum(Err(err)) => {
-                    return Err(HandlerError::Message(format!(
-                        "Parsing {} command in [comment]({}) failed: {}",
-                        stringify!($name),
-                        event.html_url().expect("has html url"),
-                        err
-                    )));
-                })*
-                Command::None => unreachable!(),
             }
-            Ok(())
         }
     }
 }

src/lib.rs

@@ -155,21 +155,34 @@ pub async fn webhook(
             return Ok(false);
         }
     };
-    if let Err(err) = handlers::handle(&ctx, &event).await {
+    let errors = handlers::handle(&ctx, &event).await;
+    let mut other_error = false;
+    let mut message = String::new();
+    for err in errors {
         match err {
-            HandlerError::Message(message) => {
-                if let Some(issue) = event.issue() {
-                    let cmnt = ErrorComment::new(issue, message);
-                    cmnt.post(&ctx.github).await?;
+            HandlerError::Message(msg) => {
+                if !message.is_empty() {
+                    message.push_str("\n\n");
                 }
+                message.push_str(&msg);
             }
             HandlerError::Other(err) => {
                 log::error!("handling event failed: {:?}", err);
-                return Err(WebhookError(anyhow::anyhow!(
-                    "handling failed, error logged",
-                )));
+                other_error = true;
             }
         }
     }
-    Ok(true)
+    if !message.is_empty() {
+        if let Some(issue) = event.issue() {
+            let cmnt = ErrorComment::new(issue, message);
+            cmnt.post(&ctx.github).await?;
+        }
+    }
+    if other_error {
+        Err(WebhookError(anyhow::anyhow!(
+            "handling failed, error logged",
+        )))
+    } else {
+        Ok(true)
+    }
 }