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.
This commit is contained in:
Graham Christensen 2017-12-22 08:51:55 -05:00
parent 4dea8198a0
commit 162cd9a982
No known key found for this signature in database
GPG key ID: ACA1C1D120C83D5C
2 changed files with 63 additions and 28 deletions

View file

@ -9,7 +9,10 @@
## Commands ## 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`. insensitive version of `@GrahamcOfBorg`.
2. To use multiple commands, insert a bit of whitespace and then your 2. To use multiple commands, insert a bit of whitespace and then your
new command. new command.
@ -61,21 +64,25 @@ or even:
@grahamcofborg build list of attrs @grahamcofborg eval @grahamcofborg build list of attrs @grahamcofborg eval
``` ```
This will _not_ work: This will also work:
``` ```
looks good to me! looks good to me!
@grahamcofborg build list of attrs @grahamcofborg build list of attrs
``` ```
Also this is bad: And this is fine:
``` ```
@grahamcofborg build list of attrs @grahamcofborg build list of attrs
looks good to me! 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? # How does OfBorg call nix-build?

View file

@ -1,5 +1,23 @@
pub fn parse(text: &str) -> Option<Vec<Instruction>> { 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() let tokens: Vec<String> = text.split_whitespace()
.map(|s| s.to_owned()).collect(); .map(|s| s.to_owned()).collect();
@ -67,6 +85,14 @@ mod tests {
assert_eq!(None, parse("")); assert_eq!(None, parse(""));
} }
#[test]
fn valid_trailing_instruction() {
assert_eq!(
Some(vec![Instruction::Eval]),
parse("/cc @grahamc for ^^
@GrahamcOfBorg eval")
);
}
#[test] #[test]
fn bogus_comment() { fn bogus_comment() {
@ -107,6 +133,31 @@ mod tests {
@grahamcofborg build foo")); @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] #[test]
fn build_and_eval_comment() { fn build_and_eval_comment() {
assert_eq!(Some(vec![ assert_eq!(Some(vec![
@ -122,8 +173,7 @@ mod tests {
fn build_comment() { fn build_comment() {
assert_eq!(Some(vec![Instruction::Build(Subset::Nixpkgs, vec![ assert_eq!(Some(vec![Instruction::Build(Subset::Nixpkgs, vec![
String::from("foo"), String::from("foo"),
String::from("bar"), String::from("bar")
String::from("baz")
])]), ])]),
parse("@GrahamCOfBorg build foo bar parse("@GrahamCOfBorg build foo bar
@ -160,28 +210,6 @@ baz"));
parse("@grahamcofborg build foo bar 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] #[test]
fn build_comment_lower_package_case_retained() { fn build_comment_lower_package_case_retained() {
assert_eq!(Some(vec![Instruction::Build(Subset::Nixpkgs, vec![ assert_eq!(Some(vec![Instruction::Build(Subset::Nixpkgs, vec![