diff --git a/CHANGELOG.md b/CHANGELOG.md index 188e1543..d9805b37 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,3 +8,5 @@ rfcbot behaves so that the people who develop rfcbot or interact with it can understand the bot better. ## Changes + ++ rfcbot now accepts multiple invocations / commands per comment you post. diff --git a/Cargo.lock b/Cargo.lock index 0def97ae..db5bf889 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -411,6 +411,11 @@ dependencies = [ "cfg-if 0.1.2 (registry+https://github.com/rust-lang/crates.io-index)", ] +[[package]] +name = "maplit" +version = "1.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" + [[package]] name = "matches" version = "0.1.6" @@ -713,6 +718,7 @@ dependencies = [ "hyper-native-tls 0.2.4 (registry+https://github.com/rust-lang/crates.io-index)", "lazy_static 0.2.11 (registry+https://github.com/rust-lang/crates.io-index)", "log 0.3.9 (registry+https://github.com/rust-lang/crates.io-index)", + "maplit 1.0.1 (registry+https://github.com/rust-lang/crates.io-index)", "r2d2 0.7.4 (registry+https://github.com/rust-lang/crates.io-index)", "r2d2-diesel 0.16.0 (registry+https://github.com/rust-lang/crates.io-index)", "rocket 0.3.8 (registry+https://github.com/rust-lang/crates.io-index)", @@ -1167,6 +1173,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" "checksum libc 0.2.40 (registry+https://github.com/rust-lang/crates.io-index)" = "6fd41f331ac7c5b8ac259b8bf82c75c0fb2e469bbf37d2becbba9a6a2221965b" "checksum log 0.3.9 (registry+https://github.com/rust-lang/crates.io-index)" = "e19e8d5c34a3e0e2223db8e060f9e8264aeeb5c5fc64a4ee9965c062211c024b" "checksum log 0.4.1 (registry+https://github.com/rust-lang/crates.io-index)" = "89f010e843f2b1a31dbd316b3b8d443758bc634bed37aabade59c686d644e0a2" +"checksum maplit 1.0.1 (registry+https://github.com/rust-lang/crates.io-index)" = "08cbb6b4fef96b6d77bfc40ec491b1690c779e77b05cd9f07f787ed376fd4c43" "checksum matches 0.1.6 (registry+https://github.com/rust-lang/crates.io-index)" = "100aabe6b8ff4e4a7e32c1c13523379802df0772b82466207ac25b013f193376" "checksum memchr 1.0.2 (registry+https://github.com/rust-lang/crates.io-index)" = "148fab2e51b4f1cfc66da2a7c32981d1d3c083a803978268bb11fe4b86925e7a" "checksum memchr 2.0.1 (registry+https://github.com/rust-lang/crates.io-index)" = "796fba70e76612589ed2ce7f45282f5af869e0fdd7cc6199fa1aa1f1d591ba9d" diff --git a/Cargo.toml b/Cargo.toml index 3b8dcb68..aad468b0 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -24,6 +24,7 @@ serde_json = "1.0" toml = "0.4" url = "1.4" urlencoded = "0.5" +maplit = "1.0.1" [dependencies.chrono] features = ["serde"] diff --git a/README.md b/README.md index 7e9cbe43..8e3433d7 100644 --- a/README.md +++ b/README.md @@ -40,6 +40,49 @@ Only the first of these commands will be registered: Examples are in each section. +### Command grammar + +rfcbot accepts roughly the following grammar: + +```ebnf +merge ::= "merge" | "merged" | "merging" | "merges" ; +close ::= "close" | "closed" | "closing" | "closes" ; +postpone ::= "postpone" | "postponed" | "postponing" | "postpones" ; +cancel ::= "cancel | "canceled" | "canceling" | "cancels" ; +review ::= "reviewed" | "review" | "reviewing" | "reviews" ; +concern ::= "concern" | "concerned" | "concerning" | "concerns" ; +resolve ::= "resolve" | "resolved" | "resolving" | "resolves" ; + +line_remainder ::= .+$ ; +ws_separated ::= ... ; + +subcommand ::= merge | close | postpone | cancel | review + | concern line_remainder + | resolve line_remainder + ; + +invocation ::= "fcp" subcommand + | "pr" subcommand + | "f?" ws_separated + | subcommand + ; + +grammar ::= "@rfcbot" ":"? invocation ; +``` + +Multiple occurrences of `grammar` are allowed in each comment you make on GitHub. +This means that the following is OK: + +``` +@rfcbot merge + +Some stuff you want to say... + +@rfcbot concern foobar + +Explain the concern... +``` + ### Final Comment Period Before proposing a final comment period on an issue/PR/RFC, please double check to make sure that the correct team label(s) has been applied to the issue. As of 9/17/16, rfcbot recognizes these labels: diff --git a/src/github/client.rs b/src/github/client.rs index b16390f5..25b4e32c 100644 --- a/src/github/client.rs +++ b/src/github/client.rs @@ -26,14 +26,6 @@ pub const DELAY: u64 = 300; type ParameterMap = BTreeMap<&'static str, String>; -macro_rules! params { - ($($key: expr => $val: expr),*) => {{ - let mut map = BTreeMap::<_, _>::new(); - $(map.insert($key, $val);)* - map - }}; -} - header! { (TZ, "Time-Zone") => [String] } header! { (Accept, "Accept") => [String] } header! { (RateLimitRemaining, "X-RateLimit-Remaining") => [u32] } @@ -94,7 +86,7 @@ impl Client { pub fn issues_since(&self, repo: &str, start: DateTime) -> DashResult> { self.get_models(&format!("{}/repos/{}/issues", BASE_URL, repo), - Some(¶ms! { + Some(&btreemap! { "state" => "all".to_string(), "since" => format!("{:?}", start), "per_page" => format!("{}", PER_PAGE), @@ -107,7 +99,7 @@ impl Client { start: DateTime) -> DashResult> { self.get_models(&format!("{}/repos/{}/issues/comments", BASE_URL, repo), - Some(¶ms! { + Some(&btreemap! { "sort" => "created".to_string(), "direction" => "asc".to_string(), "since" => format!("{:?}", start), @@ -164,7 +156,7 @@ impl Client { pub fn close_issue(&self, repo: &str, issue_num: i32) -> DashResult<()> { let url = format!("{}/repos/{}/issues/{}", BASE_URL, repo, issue_num); - let payload = serde_json::to_string(¶ms!("state" => "closed"))?; + let payload = serde_json::to_string(&btreemap!("state" => "closed"))?; let mut res = self.patch(&url, &payload)?; if StatusCode::Ok != res.status { @@ -208,9 +200,7 @@ impl Client { text: &str) -> DashResult { let url = format!("{}/repos/{}/issues/{}/comments", BASE_URL, repo, issue_num); - - let payload = serde_json::to_string(¶ms!("body" => text))?; - + let payload = serde_json::to_string(&btreemap!("body" => text))?; // FIXME propagate an error if it's a 404 or other error self.deserialize(&mut self.post(&url, &payload)?) } @@ -225,7 +215,7 @@ impl Client { repo, comment_num); - let payload = serde_json::to_string(¶ms!("body" => text))?; + let payload = serde_json::to_string(&btreemap!("body" => text))?; // FIXME propagate an error if it's a 404 or other error self.deserialize(&mut self.patch(&url, &payload)?) diff --git a/src/github/nag.rs b/src/github/nag.rs index 72247e20..1ae0e238 100644 --- a/src/github/nag.rs +++ b/src/github/nag.rs @@ -85,10 +85,13 @@ pub fn update_nags(comment: &IssueComment) -> DashResult<()> { let subteam_members = subteam_members(&issue)?; - // attempt to parse a command out of the comment - if let Ok(command) = RfcBotCommand::from_str(&comment.body) { + // Attempt to parse all commands out of the comment + let mut any = false; + for command in RfcBotCommand::from_str_all(&comment.body) { + any = true; - // don't accept bot commands from non-subteam members + // Don't accept bot commands from non-subteam members. + // Early return because we'll just get here again... if subteam_members.iter().find(|&u| u == &author).is_none() { info!("command author ({}) doesn't appear in any relevant subteams", author.login); @@ -104,8 +107,9 @@ pub fn update_nags(comment: &IssueComment) -> DashResult<()> { }); debug!("rfcbot command is processed"); + } - } else { + if !any { ok_or!(resolve_applicable_feedback_requests(&author, &issue, comment), why => error!("Unable to resolve feedback requests for comment id {}: {:?}", comment.id, why)); @@ -578,6 +582,32 @@ fn parse_command_text<'a>(command: &'a str, subcommand: &'a str) -> &'a str { /// Parses all subcommands under the fcp command. /// If `fcp_context` is set to false, `@rfcbot ` /// was passed and not `@rfcbot fcp `. +/// +/// @rfcbot accepts roughly the following grammar: +/// +/// merge ::= "merge" | "merged" | "merging" | "merges" ; +/// close ::= "close" | "closed" | "closing" | "closes" ; +/// postpone ::= "postpone" | "postponed" | "postponing" | "postpones" ; +/// cancel ::= "cancel | "canceled" | "canceling" | "cancels" ; +/// review ::= "reviewed" | "review" | "reviewing" | "reviews" ; +/// concern ::= "concern" | "concerned" | "concerning" | "concerns" ; +/// resolve ::= "resolve" | "resolved" | "resolving" | "resolves" ; +/// +/// line_remainder ::= .+$ ; +/// ws_separated ::= ... ; +/// +/// subcommand ::= merge | close | postpone | cancel | review +/// | concern line_remainder +/// | resolve line_remainder +/// ; +/// +/// invocation ::= "fcp" subcommand +/// | "pr" subcommand +/// | "f?" ws_separated +/// | subcommand +/// ; +/// +/// grammar ::= "@rfcbot" ":"? invocation ; fn parse_fcp_subcommand<'a>( command: &'a str, subcommand: &'a str, @@ -876,20 +906,20 @@ impl<'a> RfcBotCommand<'a> { Ok(()) } - pub fn from_str(command: &'a str) -> DashResult> { - // get the tokens for the command line (starts with a bot mention) - let command = command - .lines() - .find(|&l| l.starts_with(RFC_BOT_MENTION)) - .ok_or(DashError::Misc(None))? - .trim_left_matches(RFC_BOT_MENTION) - .trim_left_matches(':') - .trim(); - - let mut tokens = command.split_whitespace(); + pub fn from_str_all(command: &'a str) -> impl Iterator> { + // Get the tokens for each command line (starts with a bot mention) + command.lines() + .filter(|&l| l.starts_with(RFC_BOT_MENTION)) + .map(Self::from_invocation_line) + .filter_map(Result::ok) + } + fn from_invocation_line(command: &'a str) -> DashResult> { + let mut tokens = command.trim_left_matches(RFC_BOT_MENTION) + .trim_left_matches(':') + .trim() + .split_whitespace(); let invocation = tokens.next().ok_or(DashError::Misc(None))?; - match invocation { "fcp" | "pr" => { let subcommand = tokens.next().ok_or(DashError::Misc(None))?; @@ -913,6 +943,7 @@ impl<'a> RfcBotCommand<'a> { _ => parse_fcp_subcommand(command, invocation, false), } } + } struct RfcBotComment<'a> { @@ -1120,6 +1151,34 @@ impl<'a> RfcBotComment<'a> { mod test { use super::*; + #[test] + fn multiple_commands() { +let text = r#" +someothertext +@rfcbot: resolved CONCERN_NAME +somemoretext + +somemoretext + +@rfcbot: fcp cancel +foobar +@rfcbot concern foobar +"#; + + let cmd = RfcBotCommand::from_str_all(text).collect::>(); + assert_eq!(cmd, vec![ + RfcBotCommand::ResolveConcern("CONCERN_NAME"), + RfcBotCommand::FcpCancel, + RfcBotCommand::NewConcern("foobar"), + ]); + } + + fn ensure_take_singleton(mut iter: I) -> I::Item { + let singleton = iter.next().unwrap(); + assert!(iter.next().is_none()); + singleton + } + macro_rules! justification { () => { "\n\nSome justification here." }; } @@ -1148,8 +1207,11 @@ somemoretext") let body = concat!("@rfcbot: ", $cmd); let body_no_colon = concat!("@rfcbot ", $cmd); - let with_colon = RfcBotCommand::from_str(body).unwrap(); - let without_colon = RfcBotCommand::from_str(body_no_colon).unwrap(); + let with_colon = + ensure_take_singleton(RfcBotCommand::from_str_all(body)); + + let without_colon = + ensure_take_singleton(RfcBotCommand::from_str_all(body_no_colon)); assert_eq!(with_colon, without_colon); assert_eq!(with_colon, expected); @@ -1220,8 +1282,9 @@ somemoretext somemoretext"; - let with_colon = RfcBotCommand::from_str(body).unwrap(); - let without_colon = RfcBotCommand::from_str(body_no_colon).unwrap(); + let with_colon = ensure_take_singleton(RfcBotCommand::from_str_all(body)); + let without_colon = + ensure_take_singleton(RfcBotCommand::from_str_all(body_no_colon)); assert_eq!(with_colon, without_colon); assert_eq!(with_colon, RfcBotCommand::ResolveConcern("CONCERN_NAME")); diff --git a/src/main.rs b/src/main.rs index f180c3f2..936bfab8 100644 --- a/src/main.rs +++ b/src/main.rs @@ -30,6 +30,8 @@ extern crate serde_json; extern crate toml; extern crate url; extern crate urlencoded; +#[macro_use] +extern crate maplit; #[macro_use] mod macros;