From 162cd9a982ca697907054036fb54284d006bd6c2 Mon Sep 17 00:00:00 2001
From: Graham Christensen <graham@grahamc.com>
Date: Fri, 22 Dec 2017 08:51:55 -0500
Subject: [PATCH] Support ofborg comments inline with other text

The old design of the parser treated all whitespace the same and
mandated that `grahamcofborg` (plus the `@`) be the first token in the
text. This allowed for some ridiculous but command calls:

    grahamcofborg build foo
       bar
          baz

This used to become a build instruction for foo, bar, and baz. After
this change, it is just an instruction for building foo. This allows
for comments for people to be intertwined with comments for the bot:

    grahamcofborg build foo

    Let's see what happens!

Before this would unintentionally become a build instruction for
`foo`, `Let's`, `see`, `what`, `happens!`, and is now only going to
build `foo`.

Additionally, this comment would do nothing:

    Let's see what happens!
    grahamcofborg build foo

Or a more real case where people expected this to work:

    /cc grahamc for ^^
    GrahamcOfBorg eval

This will continue to not produce a build instruction, because
grahamcofborg must be the first word of a line:

    foo bar grahamcofborg build foo

Note: I've removed `@`s from all usernames to avoid accidental email.
---
 README.md                   | 15 ++++++--
 ofborg/src/commentparser.rs | 76 +++++++++++++++++++++++++------------
 2 files changed, 63 insertions(+), 28 deletions(-)

diff --git a/README.md b/README.md
index 5ae2111..0b443c7 100644
--- a/README.md
+++ b/README.md
@@ -9,7 +9,10 @@
 
 ## Commands
 
-1. To trigger the bot, the comment _must_ start with a case
+The comment parser is line-based, so comments can be interleaved with
+instructions.
+
+1. To trigger the bot, the line _must_ start with a case
    insensitive version of `@GrahamcOfBorg`.
 2. To use multiple commands, insert a bit of whitespace and then your
    new command.
@@ -61,21 +64,25 @@ or even:
 @grahamcofborg build list of attrs @grahamcofborg eval
 ```
 
-This will _not_ work:
+This will also work:
 
 ```
 looks good to me!
 @grahamcofborg build list of attrs
 ```
 
-Also this is bad:
+And this is fine:
 
 ```
 @grahamcofborg build list of attrs
 looks good to me!
 ```
 
-as it'll try to build `list` `of` `attrs` `looks` `good` `to` `me!`.
+This is will build `list`, `of`, `attrs`, `looks`, `good`, `to`, `me!`:
+
+```
+@grahamcofborg build list of attrs looks good to me!
+```
 
 
 # How does OfBorg call nix-build?
diff --git a/ofborg/src/commentparser.rs b/ofborg/src/commentparser.rs
index 9a5effe..526392c 100644
--- a/ofborg/src/commentparser.rs
+++ b/ofborg/src/commentparser.rs
@@ -1,5 +1,23 @@
 
 pub fn parse(text: &str) -> Option<Vec<Instruction>> {
+    let instructions: Vec<Instruction> = text.lines()
+        .map(|s| match parse_line(s) {
+            Some(instructions) => instructions,
+            None => vec![]
+        })
+        .fold(vec![], |mut collector, mut inst| {
+            collector.append(&mut inst);
+            collector
+        });
+
+    if instructions.len() == 0 {
+        return None;
+    } else {
+        return Some(instructions)
+    }
+}
+
+pub fn parse_line(text: &str) -> Option<Vec<Instruction>> {
     let tokens: Vec<String> = text.split_whitespace()
         .map(|s| s.to_owned()).collect();
 
@@ -67,6 +85,14 @@ mod tests {
         assert_eq!(None, parse(""));
     }
 
+    #[test]
+    fn valid_trailing_instruction() {
+        assert_eq!(
+            Some(vec![Instruction::Eval]),
+            parse("/cc @grahamc for ^^
+@GrahamcOfBorg eval")
+        );
+    }
 
     #[test]
     fn bogus_comment() {
@@ -107,6 +133,31 @@ mod tests {
 @grahamcofborg build foo"));
     }
 
+    #[test]
+    fn complex_comment_with_paragraphs() {
+        assert_eq!(Some(vec![
+            Instruction::Build(Subset::Nixpkgs, vec![
+                String::from("bar"),
+            ]),
+            Instruction::Eval,
+            Instruction::Build(Subset::Nixpkgs, vec![
+                String::from("foo"),
+            ])
+        ]),
+                   parse("
+I like where you're going with this PR, so let's try it out!
+
+@grahamcofborg build bar
+
+I noticed though that the target branch was broken, which should be fixed. Let's eval again.
+
+@grahamcofborg eval
+
+Also, just in case, let's try foo
+@grahamcofborg build foo"));
+    }
+
+
     #[test]
     fn build_and_eval_comment() {
         assert_eq!(Some(vec![
@@ -122,8 +173,7 @@ mod tests {
     fn build_comment() {
         assert_eq!(Some(vec![Instruction::Build(Subset::Nixpkgs, vec![
             String::from("foo"),
-            String::from("bar"),
-            String::from("baz")
+            String::from("bar")
         ])]),
                    parse("@GrahamCOfBorg build foo bar
 
@@ -160,28 +210,6 @@ baz"));
                    parse("@grahamcofborg build foo bar baz"));
     }
 
-
-    #[test]
-    fn build_whitespace_disregarded() {
-        assert_eq!(Some(vec![Instruction::Build(Subset::Nixpkgs, vec![
-            String::from("foo"),
-            String::from("bar"),
-            String::from("baz")
-        ])]),
-                   parse("
-
-
-  @grahamcofborg
-   build foo
-
-
-        bar baz
-
-"));
-    }
-
-
-
     #[test]
     fn build_comment_lower_package_case_retained() {
         assert_eq!(Some(vec![Instruction::Build(Subset::Nixpkgs, vec![