From d186d30d1d24d602fa837c8d41ec222bb7dc7604 Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Wed, 16 May 2018 01:26:06 +0200 Subject: [PATCH 01/11] initial wave of cleanup, match => if let. --- src/github/client.rs | 39 ++++++++++---------- src/github/mod.rs | 40 ++++++++------------- src/github/models.rs | 11 +++--- src/github/nag.rs | 81 ++++++++++++++++++------------------------ src/github/webhooks.rs | 26 ++++++-------- src/scraper.rs | 2 +- src/teams.rs | 10 +++--- 7 files changed, 87 insertions(+), 122 deletions(-) diff --git a/src/github/client.rs b/src/github/client.rs index fd2e0357..50676818 100644 --- a/src/github/client.rs +++ b/src/github/client.rs @@ -165,13 +165,12 @@ impl Client { let mut res = self.patch(&url, &payload)?; - match res.status { - StatusCode::Ok => Ok(()), - _ => { - let mut body = String::new(); - res.read_to_string(&mut body)?; - Err(DashError::Misc(Some(body))) - } + if let StatusCode::Ok = res.status { + Ok(()) + } else { + let mut body = String::new(); + res.read_to_string(&mut body)?; + Err(DashError::Misc(Some(body))) } } @@ -181,13 +180,12 @@ impl Client { let mut res = self.post(&url, &payload)?; - match res.status { - StatusCode::Ok => Ok(()), - _ => { - let mut body = String::new(); - res.read_to_string(&mut body)?; - Err(DashError::Misc(Some(body))) - } + if let StatusCode::Ok = res.status { + Ok(()) + } else { + let mut body = String::new(); + res.read_to_string(&mut body)?; + Err(DashError::Misc(Some(body))) } } @@ -199,13 +197,12 @@ impl Client { label); let mut res = self.delete(&url)?; - match res.status { - StatusCode::NoContent => Ok(()), - _ => { - let mut body = String::new(); - res.read_to_string(&mut body)?; - Err(DashError::Misc(Some(body))) - } + if let StatusCode::NoContent = res.status { + Ok(()) + } else { + let mut body = String::new(); + res.read_to_string(&mut body)?; + Err(DashError::Misc(Some(body))) } } diff --git a/src/github/mod.rs b/src/github/mod.rs index 999ba072..dcafe0fe 100644 --- a/src/github/mod.rs +++ b/src/github/mod.rs @@ -96,36 +96,25 @@ pub fn ingest_since(repo: &str, start: DateTime) -> DashResult<()> { // make sure we have all of the users to ensure referential integrity for issue in issues { let issue_number = issue.number; - match handle_issue(conn, issue, repo) { - Ok(()) => (), - Err(why) => { - error!("Error processing issue {}#{}: {:?}", - repo, - issue_number, - why) - } + if let Err(why) = handle_issue(conn, issue, repo) { + error!("Error processing issue {}#{}: {:?}", + repo, issue_number, why); } } // insert the comments for comment in comments { let comment_id = comment.id; - match handle_comment(conn, comment, repo) { - Ok(()) => (), - Err(why) => { - error!("Error processing comment {}#{}: {:?}", - repo, - comment_id, - why) - } + if let Err(why) = handle_comment(conn, comment, repo) { + error!("Error processing comment {}#{}: {:?}", + repo, comment_id, why); } } for pr in prs { let pr_number = pr.number; - match handle_pr(conn, pr, repo) { - Ok(()) => (), - Err(why) => error!("Error processing PR {}#{}: {:?}", repo, pr_number, why), + if let Err(why) = handle_pr(conn, pr, repo) { + error!("Error processing PR {}#{}: {:?}", repo, pr_number, why); } } @@ -159,19 +148,18 @@ pub fn handle_comment(conn: &PgConnection, comment: CommentFromJson, repo: &str) diesel::update(issuecomment::table.find(comment.id)) .set(&comment) .execute(conn)?; - Ok(()) } else { diesel::insert(&comment) .into(issuecomment::table) .execute(conn)?; - match nag::update_nags(&comment) { - Ok(()) => Ok(()), - Err(why) => { - error!("Problem updating FCPs: {:?}", &why); - Err(why) - } + + if let Err(why) = nag::update_nags(&comment) { + error!("Problem updating FCPs: {:?}", &why); + return Err(why); } } + + Ok(()) } pub fn handle_issue(conn: &PgConnection, issue: IssueFromJson, repo: &str) -> DashResult<()> { diff --git a/src/github/models.rs b/src/github/models.rs index 9e310f2c..045a0d30 100644 --- a/src/github/models.rs +++ b/src/github/models.rs @@ -129,13 +129,12 @@ impl CommentFromJson { } }; - let conn = try!(DB_POOL.get()); + let conn = DB_POOL.get()?; - let issue_id = try!(issue - .select(id) - .filter(number.eq(issue_number)) - .filter(repository.eq(repo)) - .first::(&*conn)); + let issue_id = issue.select(id) + .filter(number.eq(issue_number)) + .filter(repository.eq(repo)) + .first::(&*conn)?; Ok(IssueComment { id: self.id, diff --git a/src/github/nag.rs b/src/github/nag.rs index 23b2e92f..25e22973 100644 --- a/src/github/nag.rs +++ b/src/github/nag.rs @@ -97,35 +97,26 @@ pub fn update_nags(comment: &IssueComment) -> DashResult<()> { } debug!("processing rfcbot command: {:?}", &command); - match command.process(&author, &issue, comment, &subteam_members) { - Ok(_) => (), - Err(why) => { - error!("Unable to process command for comment id {}: {:?}", - comment.id, - why); - return Ok(()); - } - }; + if let Err(why) = + command.process(&author, &issue, comment, &subteam_members) { + error!("Unable to process command for comment id {}: {:?}", + comment.id, + why); + return Ok(()); + } debug!("rfcbot command is processed"); - } else { - match resolve_applicable_feedback_requests(&author, &issue, comment) { - Ok(_) => (), - Err(why) => { - error!("Unable to resolve feedback requests for comment id {}: {:?}", - comment.id, - why); - } - }; + } else if let Err(why) = + resolve_applicable_feedback_requests(&author, &issue, comment) { + error!("Unable to resolve feedback requests for comment id {}: {:?}", + comment.id, + why); } - match evaluate_nags() { - Ok(_) => (), - Err(why) => { - error!("Unable to evaluate outstanding proposals: {:?}", why); - } - }; + if let Err(why) = evaluate_nags() { + error!("Unable to evaluate outstanding proposals: {:?}", why); + } Ok(()) } @@ -710,12 +701,12 @@ fn parse_fcp_subcommand<'a>( }, _ => { - Err(if fcp_context { + Err(DashError::Misc(if fcp_context { error!("unrecognized subcommand for fcp: {}", subcommand); - DashError::Misc(Some(format!("found bad subcommand: {}", subcommand))) + Some(format!("found bad subcommand: {}", subcommand)) } else { - DashError::Misc(None) - }) + None + })) } } } @@ -760,14 +751,13 @@ impl<'a> RfcBotCommand<'a> { // at this point our new comment doesn't yet exist in the database, so // we need to insert it let gh_comment = gh_comment.with_repo(&issue.repository)?; - match diesel::insert(&gh_comment) + if let Err(why) = + diesel::insert(&gh_comment) .into(issuecomment::table) - .execute(conn) { - Ok(_) => {} - Err(why) => { - warn!("issue inserting new record, maybe received webhook for it: {:?}", - why); - } + .execute(conn) + { + warn!("issue inserting new record, maybe received webhook for it: {:?}", + why); } let proposal = NewFcpProposal { @@ -814,10 +804,8 @@ impl<'a> RfcBotCommand<'a> { let new_gh_comment = RfcBotComment::new(issue, - CommentType::FcpProposed(author, - disp, - &review_requests, - &[])); + CommentType::FcpProposed( + author, disp, &review_requests, &[])); new_gh_comment.post(Some(gh_comment.id))?; @@ -886,14 +874,13 @@ impl<'a> RfcBotCommand<'a> { if proposal.fcp_start.is_some() { // Update DB: FCP is not started anymore. proposal.fcp_start = None; - match diesel::update(fcp_proposal.find(proposal.id)) - .set(&proposal) - .execute(conn) { - Ok(_) => (), - Err(why) => { - error!("Unable to mark FCP {} as unstarted: {:?}", proposal.id, why); - return Ok(()); - } + if let Err(why) = + diesel::update(fcp_proposal.find(proposal.id)) + .set(&proposal) + .execute(conn) + { + error!("Unable to mark FCP {} as unstarted: {:?}", proposal.id, why); + return Ok(()); } // Update labels: diff --git a/src/github/webhooks.rs b/src/github/webhooks.rs index a7120e8e..13b1cf6d 100644 --- a/src/github/webhooks.rs +++ b/src/github/webhooks.rs @@ -46,13 +46,10 @@ impl FromData for Event { }; let mut body = String::new(); - match data.open().read_to_string(&mut body) { - Ok(_) => (), - Err(why) => { - error!("unable to read request body: {:?}", why); - return Failure((Status::InternalServerError, "unable to read request body")); - } - }; + if let Err(why) = data.open().read_to_string(&mut body) { + error!("unable to read request body: {:?}", why); + return Failure((Status::InternalServerError, "unable to read request body")); + } for secret in &CONFIG.github_webhook_secrets { if authenticate(secret, &body, signature) { @@ -96,14 +93,13 @@ impl FromData for Event { fn authenticate(secret: &str, payload: &str, signature: &str) -> bool { // https://developer.github.com/webhooks/securing/#validating-payloads-from-github let sans_prefix = signature[5..].as_bytes(); - match Vec::from_hex(sans_prefix) { - Ok(sigbytes) => { - let mut mac = Hmac::new(Sha1::new(), secret.as_bytes()); - mac.input(payload.as_bytes()); - // constant time comparison - mac.result() == MacResult::new(&sigbytes) - } - Err(_) => false, + if let Ok(sigbytes) = Vec::from_hex(sans_prefix) { + let mut mac = Hmac::new(Sha1::new(), secret.as_bytes()); + mac.input(payload.as_bytes()); + // constant time comparison + mac.result() == MacResult::new(&sigbytes) + } else { + false } } diff --git a/src/scraper.rs b/src/scraper.rs index 01be4b71..415638ed 100644 --- a/src/scraper.rs +++ b/src/scraper.rs @@ -40,7 +40,7 @@ pub fn scrape_github(since: DateTime) { let start_time = Utc::now().naive_utc(); for repo in repos { match github::ingest_since(&repo, since) { - Ok(()) => info!("Scraped {} github successfully", repo), + Ok(_) => info!("Scraped {} github successfully", repo), Err(why) => error!("Unable to scrape github {}: {:?}", repo, why), } } diff --git a/src/teams.rs b/src/teams.rs index 756795ab..42c7ab6d 100644 --- a/src/teams.rs +++ b/src/teams.rs @@ -121,15 +121,13 @@ impl Team { // bail if they don't exist, but we don't want to actually keep the id in ram for member_login in self.member_logins() { - match githubuser + if let Err(why) = + githubuser .filter(login.eq(member_login)) .first::(conn) { - Ok(_) => (), - Err(why) => { - error!("unable to find {} in database: {:?}", member_login, why); - return Err(why.into()); - } + error!("unable to find {} in database: {:?}", member_login, why); + return Err(why.into()); } } From 48ed8fd3d8fe641ddf5a879362b800ea28d6483f Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Wed, 16 May 2018 04:02:37 +0200 Subject: [PATCH 02/11] introduce a throw! macro and use it. --- src/config.rs | 8 +++----- src/github/client.rs | 42 +++++++++++++++++++++--------------------- src/github/mod.rs | 2 +- src/github/nag.rs | 24 ++++++++++++------------ src/macros.rs | 5 +++++ src/main.rs | 3 +++ src/teams.rs | 2 +- 7 files changed, 46 insertions(+), 40 deletions(-) create mode 100644 src/macros.rs diff --git a/src/config.rs b/src/config.rs index 3f3df535..e6e9c034 100644 --- a/src/config.rs +++ b/src/config.rs @@ -74,7 +74,7 @@ pub fn init() -> Result> { let db_pool_size = vars.remove(DB_POOL_SIZE).unwrap(); let db_pool_size = match db_pool_size.parse::() { Ok(size) => size, - Err(_) => return Err(vec![DB_POOL_SIZE]), + Err(_) => throw!(vec![DB_POOL_SIZE]), }; let gh_token = vars.remove(GITHUB_TOKEN).unwrap(); @@ -83,13 +83,13 @@ pub fn init() -> Result> { let gh_interval = vars.remove(GITHUB_INTERVAL).unwrap(); let gh_interval = match gh_interval.parse::() { Ok(interval) => interval, - Err(_) => return Err(vec![GITHUB_INTERVAL]), + Err(_) => throw!(vec![GITHUB_INTERVAL]), }; let post_comments = vars.remove(POST_COMMENTS).unwrap(); let post_comments = match post_comments.parse::() { Ok(pc) => pc, - Err(_) => return Err(vec![POST_COMMENTS]), + Err(_) => throw!(vec![POST_COMMENTS]), }; let webhook_secrets = vars.remove(GITHUB_WEBHOOK_SECRETS).unwrap(); @@ -106,11 +106,9 @@ pub fn init() -> Result> { }) } else { - Err(vars.iter() .filter(|&(_, v)| v.is_err()) .map(|(&k, _)| k) .collect()) - } } diff --git a/src/github/client.rs b/src/github/client.rs index 50676818..2e67501f 100644 --- a/src/github/client.rs +++ b/src/github/client.rs @@ -43,6 +43,12 @@ pub struct Client { rate_limit_timeout: DateTime, } +fn read_to_string(reader: &mut R) -> DashResult { + let mut string = String::new(); + reader.read_to_string(&mut string)?; + Ok(string) +} + impl Client { pub fn new() -> Self { let tls_connector = HttpsConnector::new(NativeTlsClient::new().unwrap()); @@ -72,7 +78,7 @@ impl Client { } } } - return Err(DashError::Misc(None)); + throw!(DashError::Misc(None)) } Ok(repos) @@ -129,7 +135,7 @@ impl Client { let mut res = try!(self.get(url, None)); self.deserialize(&mut res) } else { - Err(DashError::Misc(None)) + throw!(DashError::Misc(None)) } } @@ -165,13 +171,11 @@ impl Client { let mut res = self.patch(&url, &payload)?; - if let StatusCode::Ok = res.status { - Ok(()) - } else { - let mut body = String::new(); - res.read_to_string(&mut body)?; - Err(DashError::Misc(Some(body))) + if StatusCode::Ok != res.status { + throw!(DashError::Misc(Some(read_to_string(&mut res)?))) } + + Ok(()) } pub fn add_label(&self, repo: &str, issue_num: i32, label: &str) -> DashResult<()> { @@ -180,13 +184,11 @@ impl Client { let mut res = self.post(&url, &payload)?; - if let StatusCode::Ok = res.status { - Ok(()) - } else { - let mut body = String::new(); - res.read_to_string(&mut body)?; - Err(DashError::Misc(Some(body))) + if StatusCode::Ok != res.status { + throw!(DashError::Misc(Some(read_to_string(&mut res)?))) } + + Ok(()) } pub fn remove_label(&self, repo: &str, issue_num: i32, label: &str) -> DashResult<()> { @@ -197,13 +199,11 @@ impl Client { label); let mut res = self.delete(&url)?; - if let StatusCode::NoContent = res.status { - Ok(()) - } else { - let mut body = String::new(); - res.read_to_string(&mut body)?; - Err(DashError::Misc(Some(body))) + if StatusCode::NoContent != res.status { + throw!(DashError::Misc(Some(read_to_string(&mut res)?))) } + + Ok(()) } pub fn new_comment(&self, @@ -287,7 +287,7 @@ impl Client { Ok(m) => Ok(m), Err(why) => { error!("Unable to parse from JSON ({:?}): {}", why, buf); - Err(why.into()) + throw!(why) } } } diff --git a/src/github/mod.rs b/src/github/mod.rs index dcafe0fe..3d139780 100644 --- a/src/github/mod.rs +++ b/src/github/mod.rs @@ -155,7 +155,7 @@ pub fn handle_comment(conn: &PgConnection, comment: CommentFromJson, repo: &str) if let Err(why) = nag::update_nags(&comment) { error!("Problem updating FCPs: {:?}", &why); - return Err(why); + throw!(why); } } diff --git a/src/github/nag.rs b/src/github/nag.rs index 25e22973..7b186b7f 100644 --- a/src/github/nag.rs +++ b/src/github/nag.rs @@ -199,7 +199,7 @@ fn evaluate_nags() -> DashResult<()> { Ok(p) => p, Err(why) => { error!("Unable to retrieve list of pending proposals: {:?}", why); - return Err(why.into()); + throw!(why); } }; @@ -371,7 +371,7 @@ fn evaluate_nags() -> DashResult<()> { Err(why) => { error!("Unable to retrieve FCPs that need to be marked as finished: {:?}", why); - return Err(why.into()); + throw!(why); } }; @@ -634,12 +634,12 @@ impl FcpDisposition { } pub fn from_str(string: &str) -> DashResult { - match string { - FCP_REPR_MERGE => Ok(FcpDisposition::Merge), - FCP_REPR_CLOSE => Ok(FcpDisposition::Close), - FCP_REPR_POSTPONE => Ok(FcpDisposition::Postpone), - _ => Err(DashError::Misc(None)), - } + Ok(match string { + FCP_REPR_MERGE => FcpDisposition::Merge, + FCP_REPR_CLOSE => FcpDisposition::Close, + FCP_REPR_POSTPONE => FcpDisposition::Postpone, + _ => throw!(DashError::Misc(None)), + }) } pub fn label(self) -> Label { @@ -701,7 +701,7 @@ fn parse_fcp_subcommand<'a>( }, _ => { - Err(DashError::Misc(if fcp_context { + throw!(DashError::Misc(if fcp_context { error!("unrecognized subcommand for fcp: {}", subcommand); Some(format!("found bad subcommand: {}", subcommand)) } else { @@ -987,7 +987,7 @@ impl<'a> RfcBotCommand<'a> { .ok_or_else(|| DashError::Misc(Some("no user specified".to_string())))?; if user.is_empty() { - return Err(DashError::Misc(Some("no user specified".to_string()))); + throw!(DashError::Misc(Some("no user specified".to_string()))); } Ok(RfcBotCommand::FeedbackRequest(&user[1..])) @@ -1191,14 +1191,14 @@ impl<'a> RfcBotComment<'a> { self.issue.repository, self.issue.number); - Err(DashError::Misc(None)) + throw!(DashError::Misc(None)) } } else { info!("Skipping comment to {}#{}, comment posts are disabled.", self.issue.repository, self.issue.number); - Err(DashError::Misc(None)) + throw!(DashError::Misc(None)) } } } diff --git a/src/macros.rs b/src/macros.rs new file mode 100644 index 00000000..7dd5f230 --- /dev/null +++ b/src/macros.rs @@ -0,0 +1,5 @@ +macro_rules! throw { + ($err: expr) => { + Err::($err)? + }; +} diff --git a/src/main.rs b/src/main.rs index 095b59a1..f180c3f2 100644 --- a/src/main.rs +++ b/src/main.rs @@ -31,6 +31,9 @@ extern crate toml; extern crate url; extern crate urlencoded; +#[macro_use] +mod macros; + mod config; mod domain; mod error; diff --git a/src/teams.rs b/src/teams.rs index 42c7ab6d..bed73a95 100644 --- a/src/teams.rs +++ b/src/teams.rs @@ -127,7 +127,7 @@ impl Team { .first::(conn) { error!("unable to find {} in database: {:?}", member_login, why); - return Err(why.into()); + throw!(why); } } From 1f3a9e9af39f2b6071ffbc0140201c9b8161100c Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Wed, 16 May 2018 05:24:35 +0200 Subject: [PATCH 03/11] more semantic compression with macros. --- src/config.rs | 23 ++-- src/github/client.rs | 2 +- src/github/mod.rs | 30 ++--- src/github/nag.rs | 277 +++++++++++++++---------------------------- src/macros.rs | 18 +++ src/scraper.rs | 17 +-- src/server.rs | 4 +- src/teams.rs | 10 +- 8 files changed, 144 insertions(+), 237 deletions(-) diff --git a/src/config.rs b/src/config.rs index e6e9c034..03100b5d 100644 --- a/src/config.rs +++ b/src/config.rs @@ -71,26 +71,17 @@ pub fn init() -> Result> { .collect::>(); let db_url = vars.remove(DB_URL).unwrap(); - let db_pool_size = vars.remove(DB_POOL_SIZE).unwrap(); - let db_pool_size = match db_pool_size.parse::() { - Ok(size) => size, - Err(_) => throw!(vec![DB_POOL_SIZE]), - }; + let db_pool_size = vars.remove(DB_POOL_SIZE).unwrap().parse::(); + let db_pool_size = ok_or!(db_pool_size, throw!(vec![DB_POOL_SIZE])); let gh_token = vars.remove(GITHUB_TOKEN).unwrap(); let gh_ua = vars.remove(GITHUB_UA).unwrap(); - let gh_interval = vars.remove(GITHUB_INTERVAL).unwrap(); - let gh_interval = match gh_interval.parse::() { - Ok(interval) => interval, - Err(_) => throw!(vec![GITHUB_INTERVAL]), - }; - - let post_comments = vars.remove(POST_COMMENTS).unwrap(); - let post_comments = match post_comments.parse::() { - Ok(pc) => pc, - Err(_) => throw!(vec![POST_COMMENTS]), - }; + let gh_interval = vars.remove(GITHUB_INTERVAL).unwrap().parse::(); + let gh_interval = ok_or!(gh_interval, throw!(vec![GITHUB_INTERVAL])); + + let post_comments = vars.remove(POST_COMMENTS).unwrap().parse::(); + let post_comments = ok_or!(post_comments, throw!(vec![POST_COMMENTS])); let webhook_secrets = vars.remove(GITHUB_WEBHOOK_SECRETS).unwrap(); let webhook_secrets = webhook_secrets.split(',').map(String::from).collect(); diff --git a/src/github/client.rs b/src/github/client.rs index 2e67501f..691e8836 100644 --- a/src/github/client.rs +++ b/src/github/client.rs @@ -132,7 +132,7 @@ impl Client { let url = pr_info.get("url"); if let Some(url) = url { - let mut res = try!(self.get(url, None)); + let mut res = self.get(url, None)?; self.deserialize(&mut res) } else { throw!(DashError::Misc(None)) diff --git a/src/github/mod.rs b/src/github/mod.rs index 3d139780..239de9ac 100644 --- a/src/github/mod.rs +++ b/src/github/mod.rs @@ -70,13 +70,10 @@ pub fn ingest_since(repo: &str, start: DateTime) -> DashResult<()> { for issue in &issues { // sleep(Duration::from_millis(github::client::DELAY)); if let Some(ref pr_info) = issue.pull_request { - match GH.fetch_pull_request(pr_info) { - Ok(pr) => prs.push(pr), - Err(why) => { - error!("ERROR fetching PR info: {:?}", why); - break; - } - } + prs.push(ok_or!(GH.fetch_pull_request(pr_info), why => { + error!("ERROR fetching PR info: {:?}", why); + break; + })); } } @@ -96,26 +93,23 @@ pub fn ingest_since(repo: &str, start: DateTime) -> DashResult<()> { // make sure we have all of the users to ensure referential integrity for issue in issues { let issue_number = issue.number; - if let Err(why) = handle_issue(conn, issue, repo) { + ok_or!(handle_issue(conn, issue, repo), why => error!("Error processing issue {}#{}: {:?}", - repo, issue_number, why); - } + repo, issue_number, why)); } // insert the comments for comment in comments { let comment_id = comment.id; - if let Err(why) = handle_comment(conn, comment, repo) { + ok_or!(handle_comment(conn, comment, repo), why => error!("Error processing comment {}#{}: {:?}", - repo, comment_id, why); - } + repo, comment_id, why)); } for pr in prs { let pr_number = pr.number; - if let Err(why) = handle_pr(conn, pr, repo) { - error!("Error processing PR {}#{}: {:?}", repo, pr_number, why); - } + ok_or!(handle_pr(conn, pr, repo), why => + error!("Error processing PR {}#{}: {:?}", repo, pr_number, why)); } Ok(()) @@ -153,10 +147,10 @@ pub fn handle_comment(conn: &PgConnection, comment: CommentFromJson, repo: &str) .into(issuecomment::table) .execute(conn)?; - if let Err(why) = nag::update_nags(&comment) { + ok_or!(nag::update_nags(&comment), why => { error!("Problem updating FCPs: {:?}", &why); throw!(why); - } + }); } Ok(()) diff --git a/src/github/nag.rs b/src/github/nag.rs index 7b186b7f..72247e20 100644 --- a/src/github/nag.rs +++ b/src/github/nag.rs @@ -61,9 +61,8 @@ impl Issue { } fn close(&self) { - if let Err(why) = GH.close_issue(&self.repository, self.number) { - error!("Unable to close issue {:?}: {:?}", self, why); - } + ok_or!(GH.close_issue(&self.repository, self.number), why => + error!("Unable to close issue {:?}: {:?}", self, why)); } } @@ -97,26 +96,23 @@ pub fn update_nags(comment: &IssueComment) -> DashResult<()> { } debug!("processing rfcbot command: {:?}", &command); - if let Err(why) = - command.process(&author, &issue, comment, &subteam_members) { + let process = command.process(&author, &issue, comment, &subteam_members); + ok_or!(process, why => { error!("Unable to process command for comment id {}: {:?}", - comment.id, - why); + comment.id, why); return Ok(()); - } + }); debug!("rfcbot command is processed"); - } else if let Err(why) = - resolve_applicable_feedback_requests(&author, &issue, comment) { - error!("Unable to resolve feedback requests for comment id {}: {:?}", - comment.id, - why); + } else { + ok_or!(resolve_applicable_feedback_requests(&author, &issue, comment), + why => error!("Unable to resolve feedback requests for comment id {}: {:?}", + comment.id, why)); } - if let Err(why) = evaluate_nags() { - error!("Unable to evaluate outstanding proposals: {:?}", why); - } + ok_or!(evaluate_nags(), why => + error!("Unable to evaluate outstanding proposals: {:?}", why)); Ok(()) } @@ -193,84 +189,45 @@ fn evaluate_nags() -> DashResult<()> { let conn = &*DB_POOL.get()?; // first process all "pending" proposals (unreviewed or remaining concerns) - let pending_proposals = match fcp_proposal - .filter(fcp_start.is_null()) - .load::(conn) { - Ok(p) => p, - Err(why) => { - error!("Unable to retrieve list of pending proposals: {:?}", why); - throw!(why); - } - }; + let pending = fcp_proposal.filter(fcp_start.is_null()).load::(conn); + let pending_proposals = ok_or!(pending, why => { + error!("Unable to retrieve list of pending proposals: {:?}", why); + throw!(why) + }); for mut proposal in pending_proposals { - let initiator = match githubuser::table - .find(proposal.fk_initiator) - .first::(conn) { - Ok(i) => i, - Err(why) => { - error!("Unable to retrieve proposal initiator for proposal id {}: {:?}", - proposal.id, - why); - continue; - } - }; + let initiator = githubuser::table.find(proposal.fk_initiator) + .first::(conn); + let initiator = ok_or_continue!(initiator, why => + error!("Unable to retrieve proposal initiator for proposal id {}: {:?}", + proposal.id, why)); - let issue = match issue::table.find(proposal.fk_issue).first::(conn) { - Ok(i) => i, - Err(why) => { - error!("Unable to retrieve issue for proposal {}: {:?}", - proposal.id, - why); - continue; - } - }; + let issue = issue::table.find(proposal.fk_issue).first::(conn); + let issue = ok_or_continue!(issue, why => + error!("Unable to retrieve issue for proposal {}: {:?}", + proposal.id, why)); // if the issue has been closed before an FCP starts, // then we just need to cancel the FCP entirely if !issue.open { - match cancel_fcp(&initiator, &issue, &proposal) { - Ok(_) => (), - Err(why) => { - error!("Unable to cancel FCP for proposal {}: {:?}", - proposal.id, - why); - continue; - } - }; + ok_or_continue!(cancel_fcp(&initiator, &issue, &proposal), why => + error!("Unable to cancel FCP for proposal {}: {:?}", + proposal.id, why)); } // check to see if any checkboxes were modified before we end up replacing the comment - match update_proposal_review_status(proposal.id) { - Ok(_) => (), - Err(why) => { - error!("Unable to update review status for proposal {}: {:?}", - proposal.id, - why); - continue; - } - } + ok_or_continue!(update_proposal_review_status(proposal.id), why => + error!("Unable to update review status for proposal {}: {:?}", + proposal.id, why)); // get associated concerns and reviews - let reviews = match list_review_requests(proposal.id) { - Ok(r) => r, - Err(why) => { - error!("Unable to retrieve review requests for proposal {}: {:?}", - proposal.id, - why); - continue; - } - }; + let reviews = ok_or_continue!(list_review_requests(proposal.id), why => + error!("Unable to retrieve review requests for proposal {}: {:?}", + proposal.id, why)); - let concerns = match list_concerns_with_authors(proposal.id) { - Ok(c) => c, - Err(why) => { - error!("Unable to retrieve concerns for proposal {}: {:?}", - proposal.id, - why); - continue; - } - }; + let concerns = ok_or_continue!(list_concerns_with_authors(proposal.id), + why => error!("Unable to retrieve concerns for proposal {}: {:?}", + proposal.id, why)); let num_outstanding_reviews = reviews.iter().filter(|&&(_, ref r)| !r.reviewed).count(); let num_complete_reviews = reviews.len() - num_outstanding_reviews; @@ -294,15 +251,10 @@ fn evaluate_nags() -> DashResult<()> { // if the comment body in the database equals the new one we generated, then no change // is needed from github (this assumes our DB accurately reflects GH's, which should // be true in most cases by the time this is called) - match status_comment.post(Some(proposal.fk_bot_tracking_comment)) { - Ok(_) => (), - Err(why) => { - error!("Unable to update status comment for proposal {}: {:?}", - proposal.id, - why); - continue; - } - }; + let post = status_comment.post(Some(proposal.fk_bot_tracking_comment)); + ok_or_continue!(post, why => + error!("Unable to update status comment for proposal {}: {:?}", + proposal.id, why)); } let majority_complete = num_outstanding_reviews < num_complete_reviews; @@ -313,15 +265,11 @@ fn evaluate_nags() -> DashResult<()> { // FCP can start now -- update the database proposal.fcp_start = Some(Utc::now().naive_utc()); - match diesel::update(fcp_proposal.find(proposal.id)) - .set(&proposal) - .execute(conn) { - Ok(_) => (), - Err(why) => { - error!("Unable to mark FCP {} as started: {:?}", proposal.id, why); - continue; - } - } + let update = diesel::update(fcp_proposal.find(proposal.id)) + .set(&proposal).execute(conn); + ok_or_continue!(update, why => + error!("Unable to mark FCP {} as started: {:?}", + proposal.id, why)); // attempt to add the final-comment-period label // TODO only add label if FCP > 1 day @@ -348,69 +296,45 @@ fn evaluate_nags() -> DashResult<()> { // leave a comment for FCP start let fcp_start_comment = RfcBotComment::new(&issue, comment_type); - match fcp_start_comment.post(None) { - Ok(_) => (), - Err(why) => { - error!("Unable to post comment for FCP {}'s start: {:?}", - proposal.id, - why); - continue; - } - }; + ok_or_continue!(fcp_start_comment.post(None), why => + error!("Unable to post comment for FCP {}'s start: {:?}", + proposal.id, why)); } } } // look for any FCP proposals that entered FCP a week or more ago but aren't marked as closed let one_business_week_ago = Utc::now().naive_utc() - Duration::days(10); - let finished_fcps = match fcp_proposal - .filter(fcp_start.le(one_business_week_ago)) - .filter(fcp_closed.eq(false)) - .load::(conn) { - Ok(f) => f, - Err(why) => { - error!("Unable to retrieve FCPs that need to be marked as finished: {:?}", - why); - throw!(why); - } - }; + let ffcps = fcp_proposal.filter(fcp_start.le(one_business_week_ago)) + .filter(fcp_closed.eq(false)) + .load::(conn); + let finished_fcps = ok_or!(ffcps, why => { + error!("Unable to retrieve FCPs that need to be marked as finished: {:?}", + why); + throw!(why); + }); for mut proposal in finished_fcps { - let initiator = match githubuser::table - .find(proposal.fk_initiator) - .first::(conn) { - Ok(i) => i, - Err(why) => { - error!("Unable to retrieve proposal initiator for proposal id {}: {:?}", - proposal.id, - why); - continue; - } - }; - - let issue = match issue::table.find(proposal.fk_issue).first::(conn) { - Ok(i) => i, - Err(why) => { - error!("Unable to find issue to match proposal {}: {:?}", - proposal.id, - why); - continue; - } - }; + let initiator = githubuser::table.find(proposal.fk_initiator) + .first::(conn); + let initiator = ok_or_continue!(initiator, why => + error!("Unable to retrieve proposal initiator for proposal id {}: {:?}", + proposal.id, + why)); + + let issue = issue::table.find(proposal.fk_issue).first::(conn); + let issue = ok_or_continue!(issue, why => + error!("Unable to find issue to match proposal {}: {:?}", + proposal.id, why)); // TODO only update the db if the comment posts, but reconcile if we find out it worked // update the fcp proposal.fcp_closed = true; - match diesel::update(fcp_proposal.find(proposal.id)) - .set(&proposal) - .execute(conn) { - Ok(_) => (), - Err(why) => { - error!("Unable to update FCP {}: {:?}", proposal.id, why); - continue; - } - } + let update_fcp = diesel::update(fcp_proposal.find(proposal.id)) + .set(&proposal).execute(conn); + ok_or_continue!(update_fcp, why => + error!("Unable to update FCP {}: {:?}", proposal.id, why)); // parse the disposition: let disp = FcpDisposition::from_str(&proposal.disposition)?; @@ -419,7 +343,7 @@ fn evaluate_nags() -> DashResult<()> { let label_res = issue.add_label(Label::FFCP); issue.remove_label(Label::FCP); let added_label = match label_res { - Ok(()) => true, + Ok(_) => true, Err(why) => { warn!("Unable to add Finished-FCP label to {}#{}: {:?}", &issue.repository, @@ -439,15 +363,9 @@ fn evaluate_nags() -> DashResult<()> { let fcp_close_comment = RfcBotComment::new(&issue, comment_type); // Post it! - match fcp_close_comment.post(None) { - Ok(_) => (), - Err(why) => { - error!("Unable to post FCP-ending comment for proposal {}: {:?}", - proposal.id, - why); - continue; - } - }; + ok_or_continue!(fcp_close_comment.post(None), why => + error!("Unable to post FCP-ending comment for proposal {}: {:?}", + proposal.id, why)); execute_ffcp_actions(&issue, disp); } @@ -665,39 +583,39 @@ fn parse_fcp_subcommand<'a>( subcommand: &'a str, fcp_context: bool ) -> DashResult> { - match subcommand { + Ok(match subcommand { // Parse a FCP merge command: "merge" | "merged" | "merging" | "merges" => - Ok(RfcBotCommand::FcpPropose(FcpDisposition::Merge)), + RfcBotCommand::FcpPropose(FcpDisposition::Merge), // Parse a FCP close command: "close" | "closed" | "closing" | "closes" => - Ok(RfcBotCommand::FcpPropose(FcpDisposition::Close)), + RfcBotCommand::FcpPropose(FcpDisposition::Close), // Parse a FCP postpone command: "postpone" | "postponed" | "postponing" | "postpones" => - Ok(RfcBotCommand::FcpPropose(FcpDisposition::Postpone)), + RfcBotCommand::FcpPropose(FcpDisposition::Postpone), // Parse a FCP cancel command: "cancel" | "canceled" | "canceling" | "cancels" => - Ok(RfcBotCommand::FcpCancel), + RfcBotCommand::FcpCancel, // Parse a FCP reviewed command: "reviewed" | "review" | "reviewing" | "reviews" => - Ok(RfcBotCommand::Reviewed), + RfcBotCommand::Reviewed, // Parse a FCP concern command: "concern" | "concerned" | "concerning" | "concerns" => { debug!("Parsed command as NewConcern"); let what = parse_command_text(command, subcommand); - Ok(RfcBotCommand::NewConcern(what)) + RfcBotCommand::NewConcern(what) }, // Parse a FCP resolve command: "resolve" | "resolved" | "resolving" | "resolves" => { debug!("Parsed command as ResolveConcern"); let what = parse_command_text(command, subcommand); - Ok(RfcBotCommand::ResolveConcern(what)) + RfcBotCommand::ResolveConcern(what) }, _ => { @@ -708,7 +626,7 @@ fn parse_fcp_subcommand<'a>( None })) } - } + }) } impl<'a> RfcBotCommand<'a> { @@ -874,14 +792,14 @@ impl<'a> RfcBotCommand<'a> { if proposal.fcp_start.is_some() { // Update DB: FCP is not started anymore. proposal.fcp_start = None; - if let Err(why) = + let update = diesel::update(fcp_proposal.find(proposal.id)) .set(&proposal) - .execute(conn) - { + .execute(conn); + ok_or!(update, why => { error!("Unable to mark FCP {} as unstarted: {:?}", proposal.id, why); return Ok(()); - } + }); // Update labels: let _ = issue.add_label(Label::PFCP); @@ -1175,16 +1093,12 @@ impl<'a> RfcBotComment<'a> { use config::CONFIG; if CONFIG.post_comments { - if self.issue.open { - match existing_comment { - Some(comment_id) => { - self.maybe_add_pfcp_label(); - GH.edit_comment(&self.issue.repository, comment_id, &self.body) - } - None => { - GH.new_comment(&self.issue.repository, self.issue.number, &self.body) - } + if let Some(comment_id) = existing_comment { + self.maybe_add_pfcp_label(); + GH.edit_comment(&self.issue.repository, comment_id, &self.body) + } else { + GH.new_comment(&self.issue.repository, self.issue.number, &self.body) } } else { info!("Skipping comment to {}#{}, the issue is no longer open", @@ -1193,7 +1107,6 @@ impl<'a> RfcBotComment<'a> { throw!(DashError::Misc(None)) } - } else { info!("Skipping comment to {}#{}, comment posts are disabled.", self.issue.repository, diff --git a/src/macros.rs b/src/macros.rs index 7dd5f230..45b6c25b 100644 --- a/src/macros.rs +++ b/src/macros.rs @@ -1,3 +1,21 @@ +macro_rules! ok_or { + ($test: expr, $on_err: expr) => { + ok_or!($test, _e => $on_err); + }; + ($test: expr, $why: ident => $on_err: expr) => { + match $test { + Ok(ok) => ok, + Err($why) => $on_err, + }; + }; +} + +macro_rules! ok_or_continue { + ($test: expr, $why: ident => $on_err: expr) => { + ok_or!($test, $why => { $on_err; continue; }) + }; +} + macro_rules! throw { ($err: expr) => { Err::($err)? diff --git a/src/scraper.rs b/src/scraper.rs index 415638ed..12be988b 100644 --- a/src/scraper.rs +++ b/src/scraper.rs @@ -27,13 +27,10 @@ pub fn start_scraping() -> JoinHandle<()> { pub fn scrape_github(since: DateTime) { let mut repos = Vec::new(); for org in &GH_ORGS { - match github::GH.org_repos(org) { - Ok(r) => repos.extend(r), - Err(why) => { - error!("Unable to retrieve repos for {}: {:?}", org, why); - return; - } - } + repos.extend(ok_or!(github::GH.org_repos(org), why => { + error!("Unable to retrieve repos for {}: {:?}", org, why); + return; + })); } info!("Scraping github activity since {:?}", since); @@ -45,8 +42,6 @@ pub fn scrape_github(since: DateTime) { } } - match github::record_successful_update(start_time) { - Ok(_) => {} - Err(why) => error!("Problem recording successful update: {:?}", why), - } + ok_or!(github::record_successful_update(start_time), why => + error!("Problem recording successful update: {:?}", why)); } diff --git a/src/server.rs b/src/server.rs index a55cb449..d4e01f83 100644 --- a/src/server.rs +++ b/src/server.rs @@ -19,9 +19,7 @@ pub fn serve() { .launch(); }); - if let Err(why) = result { - error!("Rocket failed to ignite: {:?}", why); - } + ok_or!(result, why => error!("Rocket failed to ignite: {:?}", why)); } } diff --git a/src/teams.rs b/src/teams.rs index bed73a95..2e92d1cf 100644 --- a/src/teams.rs +++ b/src/teams.rs @@ -121,14 +121,12 @@ impl Team { // bail if they don't exist, but we don't want to actually keep the id in ram for member_login in self.member_logins() { - if let Err(why) = - githubuser - .filter(login.eq(member_login)) - .first::(conn) - { + let check_login = githubuser.filter(login.eq(member_login)) + .first::(conn); + ok_or!(check_login, why => { error!("unable to find {} in database: {:?}", member_login, why); throw!(why); - } + }); } Ok(()) From 04debe6e124f8d68c335f1ee98e1395b3b54808e Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Wed, 23 May 2018 02:33:27 +0200 Subject: [PATCH 04/11] start of a changelog. --- CHANGELOG.md | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 CHANGELOG.md diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 00000000..188e1543 --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,10 @@ +# CHANGELOG + +`@rfcbot` is Rust's automated process manager for RFCs, tracking issues, etc. +As such, this application does not follow semver of any form. + +However, we do maintain this changelog explaining high level changes to the way +rfcbot behaves so that the people who develop rfcbot or interact with it can +understand the bot better. + +## Changes From 767cd0312a04e233e59eb90d8ecb362c8ba62f8d Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Wed, 23 May 2018 02:49:58 +0200 Subject: [PATCH 05/11] serde -> v1.0.59, use #[serde(transparent)] --- Cargo.lock | 57 +++++++++++++++++++++------------------------------- Cargo.toml | 4 ++-- src/teams.rs | 16 ++------------- 3 files changed, 27 insertions(+), 50 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index bbc73804..0def97ae 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -72,7 +72,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" dependencies = [ "num-integer 0.1.36 (registry+https://github.com/rust-lang/crates.io-index)", "num-traits 0.2.2 (registry+https://github.com/rust-lang/crates.io-index)", - "serde 1.0.41 (registry+https://github.com/rust-lang/crates.io-index)", + "serde 1.0.59 (registry+https://github.com/rust-lang/crates.io-index)", "time 0.1.39 (registry+https://github.com/rust-lang/crates.io-index)", ] @@ -276,7 +276,7 @@ dependencies = [ "pest 0.3.3 (registry+https://github.com/rust-lang/crates.io-index)", "quick-error 1.2.1 (registry+https://github.com/rust-lang/crates.io-index)", "regex 0.2.10 (registry+https://github.com/rust-lang/crates.io-index)", - "serde 1.0.41 (registry+https://github.com/rust-lang/crates.io-index)", + "serde 1.0.59 (registry+https://github.com/rust-lang/crates.io-index)", "serde_json 1.0.15 (registry+https://github.com/rust-lang/crates.io-index)", ] @@ -582,7 +582,7 @@ dependencies = [ [[package]] name = "proc-macro2" -version = "0.3.6" +version = "0.4.3" source = "registry+https://github.com/rust-lang/crates.io-index" dependencies = [ "unicode-xid 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)", @@ -600,10 +600,10 @@ source = "registry+https://github.com/rust-lang/crates.io-index" [[package]] name = "quote" -version = "0.5.1" +version = "0.6.2" source = "registry+https://github.com/rust-lang/crates.io-index" dependencies = [ - "proc-macro2 0.3.6 (registry+https://github.com/rust-lang/crates.io-index)", + "proc-macro2 0.4.3 (registry+https://github.com/rust-lang/crates.io-index)", ] [[package]] @@ -719,8 +719,8 @@ dependencies = [ "rocket_codegen 0.3.8 (registry+https://github.com/rust-lang/crates.io-index)", "rocket_contrib 0.3.8 (registry+https://github.com/rust-lang/crates.io-index)", "rust-crypto 0.2.36 (registry+https://github.com/rust-lang/crates.io-index)", - "serde 1.0.41 (registry+https://github.com/rust-lang/crates.io-index)", - "serde_derive 1.0.41 (registry+https://github.com/rust-lang/crates.io-index)", + "serde 1.0.59 (registry+https://github.com/rust-lang/crates.io-index)", + "serde_derive 1.0.59 (registry+https://github.com/rust-lang/crates.io-index)", "serde_json 1.0.15 (registry+https://github.com/rust-lang/crates.io-index)", "toml 0.4.6 (registry+https://github.com/rust-lang/crates.io-index)", "url 1.7.0 (registry+https://github.com/rust-lang/crates.io-index)", @@ -781,7 +781,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" dependencies = [ "log 0.3.9 (registry+https://github.com/rust-lang/crates.io-index)", "rocket 0.3.8 (registry+https://github.com/rust-lang/crates.io-index)", - "serde 1.0.41 (registry+https://github.com/rust-lang/crates.io-index)", + "serde 1.0.59 (registry+https://github.com/rust-lang/crates.io-index)", "serde_json 1.0.15 (registry+https://github.com/rust-lang/crates.io-index)", ] @@ -856,27 +856,17 @@ source = "registry+https://github.com/rust-lang/crates.io-index" [[package]] name = "serde" -version = "1.0.41" +version = "1.0.59" source = "registry+https://github.com/rust-lang/crates.io-index" [[package]] name = "serde_derive" -version = "1.0.41" -source = "registry+https://github.com/rust-lang/crates.io-index" -dependencies = [ - "proc-macro2 0.3.6 (registry+https://github.com/rust-lang/crates.io-index)", - "quote 0.5.1 (registry+https://github.com/rust-lang/crates.io-index)", - "serde_derive_internals 0.23.1 (registry+https://github.com/rust-lang/crates.io-index)", - "syn 0.13.1 (registry+https://github.com/rust-lang/crates.io-index)", -] - -[[package]] -name = "serde_derive_internals" -version = "0.23.1" +version = "1.0.59" source = "registry+https://github.com/rust-lang/crates.io-index" dependencies = [ - "proc-macro2 0.3.6 (registry+https://github.com/rust-lang/crates.io-index)", - "syn 0.13.1 (registry+https://github.com/rust-lang/crates.io-index)", + "proc-macro2 0.4.3 (registry+https://github.com/rust-lang/crates.io-index)", + "quote 0.6.2 (registry+https://github.com/rust-lang/crates.io-index)", + "syn 0.14.0 (registry+https://github.com/rust-lang/crates.io-index)", ] [[package]] @@ -898,7 +888,7 @@ dependencies = [ "dtoa 0.4.2 (registry+https://github.com/rust-lang/crates.io-index)", "itoa 0.4.1 (registry+https://github.com/rust-lang/crates.io-index)", "num-traits 0.2.2 (registry+https://github.com/rust-lang/crates.io-index)", - "serde 1.0.41 (registry+https://github.com/rust-lang/crates.io-index)", + "serde 1.0.59 (registry+https://github.com/rust-lang/crates.io-index)", ] [[package]] @@ -923,11 +913,11 @@ dependencies = [ [[package]] name = "syn" -version = "0.13.1" +version = "0.14.0" source = "registry+https://github.com/rust-lang/crates.io-index" dependencies = [ - "proc-macro2 0.3.6 (registry+https://github.com/rust-lang/crates.io-index)", - "quote 0.5.1 (registry+https://github.com/rust-lang/crates.io-index)", + "proc-macro2 0.4.3 (registry+https://github.com/rust-lang/crates.io-index)", + "quote 0.6.2 (registry+https://github.com/rust-lang/crates.io-index)", "unicode-xid 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)", ] @@ -972,7 +962,7 @@ name = "toml" version = "0.4.6" source = "registry+https://github.com/rust-lang/crates.io-index" dependencies = [ - "serde 1.0.41 (registry+https://github.com/rust-lang/crates.io-index)", + "serde 1.0.59 (registry+https://github.com/rust-lang/crates.io-index)", ] [[package]] @@ -1200,10 +1190,10 @@ source = "registry+https://github.com/rust-lang/crates.io-index" "checksum pkg-config 0.3.9 (registry+https://github.com/rust-lang/crates.io-index)" = "3a8b4c6b8165cd1a1cd4b9b120978131389f64bdaf456435caa41e630edba903" "checksum plugin 0.2.6 (registry+https://github.com/rust-lang/crates.io-index)" = "1a6a0dc3910bc8db877ffed8e457763b317cf880df4ae19109b9f77d277cf6e0" "checksum pq-sys 0.4.4 (registry+https://github.com/rust-lang/crates.io-index)" = "4dfb5e575ef93a1b7b2a381d47ba7c5d4e4f73bff37cee932195de769aad9a54" -"checksum proc-macro2 0.3.6 (registry+https://github.com/rust-lang/crates.io-index)" = "49b6a521dc81b643e9a51e0d1cf05df46d5a2f3c0280ea72bcb68276ba64a118" +"checksum proc-macro2 0.4.3 (registry+https://github.com/rust-lang/crates.io-index)" = "a45f2f0ae0b5757f6fe9e68745ba25f5246aea3598984ed81d013865873c1f84" "checksum quick-error 1.2.1 (registry+https://github.com/rust-lang/crates.io-index)" = "eda5fe9b71976e62bc81b781206aaa076401769b2143379d3eb2118388babac4" "checksum quote 0.3.15 (registry+https://github.com/rust-lang/crates.io-index)" = "7a6e920b65c65f10b2ae65c831a81a073a89edd28c7cce89475bff467ab4167a" -"checksum quote 0.5.1 (registry+https://github.com/rust-lang/crates.io-index)" = "7b0ff51282f28dc1b53fd154298feaa2e77c5ea0dba68e1fd8b03b72fbe13d2a" +"checksum quote 0.6.2 (registry+https://github.com/rust-lang/crates.io-index)" = "9e53eeda07ddbd8b057dde66d9beded11d0dfda13f0db0769e6b71d6bcf2074e" "checksum r2d2 0.7.4 (registry+https://github.com/rust-lang/crates.io-index)" = "2c8284508b38df440f8f3527395e23c4780b22f74226b270daf58fee38e4bcce" "checksum r2d2-diesel 0.16.0 (registry+https://github.com/rust-lang/crates.io-index)" = "f6b921696a6c45991296d21b52ed973b9fb56f6c47524fda1f99458c2d6c0478" "checksum rand 0.3.22 (registry+https://github.com/rust-lang/crates.io-index)" = "15a732abf9d20f0ad8eeb6f909bf6868722d9a06e1e50802b6a70351f40b4eb1" @@ -1227,15 +1217,14 @@ source = "registry+https://github.com/rust-lang/crates.io-index" "checksum security-framework 0.1.16 (registry+https://github.com/rust-lang/crates.io-index)" = "dfa44ee9c54ce5eecc9de7d5acbad112ee58755239381f687e564004ba4a2332" "checksum security-framework-sys 0.1.16 (registry+https://github.com/rust-lang/crates.io-index)" = "5421621e836278a0b139268f36eee0dc7e389b784dc3f79d8f11aabadf41bead" "checksum serde 0.8.23 (registry+https://github.com/rust-lang/crates.io-index)" = "9dad3f759919b92c3068c696c15c3d17238234498bbdcc80f2c469606f948ac8" -"checksum serde 1.0.41 (registry+https://github.com/rust-lang/crates.io-index)" = "5f751e9d57bd42502e4362b2d84f916ed9578e9a1a46852dcdeb6f91f6de7c14" -"checksum serde_derive 1.0.41 (registry+https://github.com/rust-lang/crates.io-index)" = "81ec46cb12594da6750ad5a913f8175798c371d6c5ffd07f082ac4fea32789c3" -"checksum serde_derive_internals 0.23.1 (registry+https://github.com/rust-lang/crates.io-index)" = "9d30c4596450fd7bbda79ef15559683f9a79ac0193ea819db90000d7e1cae794" +"checksum serde 1.0.59 (registry+https://github.com/rust-lang/crates.io-index)" = "2a4d976362a13caad61c38cf841401d2d4d480496a9391c3842c288b01f9de95" +"checksum serde_derive 1.0.59 (registry+https://github.com/rust-lang/crates.io-index)" = "94bb618afe46430c6b089e9b111dc5b2fcd3e26a268da0993f6d16bea51c6021" "checksum serde_json 0.8.6 (registry+https://github.com/rust-lang/crates.io-index)" = "67f7d2e9edc3523a9c8ec8cd6ec481b3a27810aafee3e625d311febd3e656b4c" "checksum serde_json 1.0.15 (registry+https://github.com/rust-lang/crates.io-index)" = "7bf1cbb1387028a13739cb018ee0d9b3db534f22ca3c84a5904f7eadfde14e75" "checksum smallvec 0.4.4 (registry+https://github.com/rust-lang/crates.io-index)" = "ee4f357e8cd37bf8822e1b964e96fd39e2cb5a0424f8aaa284ccaccc2162411c" "checksum state 0.4.0 (registry+https://github.com/rust-lang/crates.io-index)" = "d5562ac59585fe3d9a1ccf6b4e298ce773f5063db80d59f783776b410c1714c2" "checksum syn 0.11.11 (registry+https://github.com/rust-lang/crates.io-index)" = "d3b891b9015c88c576343b9b3e41c2c11a51c219ef067b264bd9c8aa9b441dad" -"checksum syn 0.13.1 (registry+https://github.com/rust-lang/crates.io-index)" = "91b52877572087400e83d24b9178488541e3d535259e04ff17a63df1e5ceff59" +"checksum syn 0.14.0 (registry+https://github.com/rust-lang/crates.io-index)" = "99d991a9e7c33123925e511baab68f7ec25c3795962fe326a2395e5a42a614f0" "checksum synom 0.11.3 (registry+https://github.com/rust-lang/crates.io-index)" = "a393066ed9010ebaed60b9eafa373d4b1baac186dd7e008555b0f702b51945b6" "checksum tempdir 0.3.7 (registry+https://github.com/rust-lang/crates.io-index)" = "15f2b5fb00ccdf689e0149d1b1b3c03fead81c2b37735d812fa8bddbbf41b6d8" "checksum thread_local 0.3.5 (registry+https://github.com/rust-lang/crates.io-index)" = "279ef31c19ededf577bfd12dfae728040a21f635b06a24cd670ff510edd38963" diff --git a/Cargo.toml b/Cargo.toml index 73560eaf..3b8dcb68 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -18,8 +18,8 @@ rocket = "0.3.3" rocket_codegen = "0.3.3" rocket_contrib = "0.3.3" rust-crypto = "0.2.36" -serde = "1.0" -serde_derive = "1.0" +serde = "1.0.59" +serde_derive = "1.0.59" serde_json = "1.0" toml = "0.4" url = "1.4" diff --git a/src/teams.rs b/src/teams.rs index 2e92d1cf..d7f9128b 100644 --- a/src/teams.rs +++ b/src/teams.rs @@ -4,7 +4,6 @@ use std::collections::BTreeMap; use diesel::prelude::*; use toml; -use serde::de; use super::DB_POOL; use domain::github::GitHubUser; @@ -72,21 +71,10 @@ impl Team { } } -#[derive(Debug, Eq, Hash, Ord, PartialEq, PartialOrd)] +#[derive(Debug, Eq, Hash, Ord, PartialEq, PartialOrd, Deserialize)] +#[serde(transparent)] pub struct TeamLabel(pub String); -impl<'de> de::Deserialize<'de> for TeamLabel { - fn deserialize>(de: D) -> Result { - String::deserialize(de).map(TeamLabel) - } - - fn deserialize_in_place>(de: D, place: &mut Self) - -> Result<(), D::Error> - { - String::deserialize_in_place(de, &mut place.0) - } -} - //============================================================================== // Implementation details //============================================================================== From 4edc03cc3539b1b32aa44b70c13601a9e0eddc25 Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Wed, 23 May 2018 05:10:08 +0200 Subject: [PATCH 06/11] implement control over reaction prohibition. --- rfcbot.toml | 18 ++++++++++ src/domain/github.rs | 11 ++++++ src/teams.rs | 85 ++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 108 insertions(+), 6 deletions(-) diff --git a/rfcbot.toml b/rfcbot.toml index def9665c..8e92efbc 100644 --- a/rfcbot.toml +++ b/rfcbot.toml @@ -1,3 +1,21 @@ +[prohibited_reactions] + +[prohibited_reactions."rust-lang/rfcs".issue] +down_vote = true +confused = true + +[prohibited_reactions."rust-lang/rfcs".comment] +down_vote = true +confused = true + +[prohibited_reactions."rust-lang/rust".issue] +down_vote = true +confused = true + +[prohibited_reactions."rust-lang/rust".comment] +down_vote = true +confused = true + [fcp_behaviors] [fcp_behaviors."rust-lang/rfcs"] diff --git a/src/domain/github.rs b/src/domain/github.rs index 567b902a..0f699695 100644 --- a/src/domain/github.rs +++ b/src/domain/github.rs @@ -149,3 +149,14 @@ pub struct PullRequest { pub changed_files: i32, pub repository: String, } + +#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Debug)] +pub enum Reaction { + Unknown, + Upvote, + Downvote, + Laugh, + Hooray, + Confused, + Heart, +} diff --git a/src/teams.rs b/src/teams.rs index d7f9128b..7ba2030a 100644 --- a/src/teams.rs +++ b/src/teams.rs @@ -6,7 +6,7 @@ use diesel::prelude::*; use toml; use super::DB_POOL; -use domain::github::GitHubUser; +use domain::github::{Reaction, GitHubUser}; use error::*; //============================================================================== @@ -17,8 +17,9 @@ lazy_static! { pub static ref SETUP: RfcbotConfig = read_rfcbot_cfg_validated(); } -#[derive(Debug, Deserialize)] +#[derive(Debug, Deserialize, Serialize)] pub struct RfcbotConfig { + prohibited_reactions: BTreeMap, fcp_behaviors: BTreeMap, teams: BTreeMap, } @@ -43,17 +44,70 @@ impl RfcbotConfig { pub fn should_ffcp_auto_postpone(&self, repo: &str) -> bool { self.fcp_behaviors.get(repo).map(|fcp| fcp.postpone).unwrap_or_default() } + + /// Is the reaction permitted on issues / PRs of this repository? + pub fn is_issue_reaction_probihited(&self, repo: &str, reaction: Reaction) + -> bool + { + self.prohibited_reactions + .get(repo) + .map(|rb| rb.issue.is_reaction_prohibited(reaction)) + .unwrap_or_default() + } + + /// Is the reaction permitted on comments of issues / PRs of this repository? + pub fn is_comment_reaction_probihited(&self, repo: &str, reaction: Reaction) + -> bool + { + self.prohibited_reactions + .get(repo) + .map(|rb| rb.comment.is_reaction_prohibited(reaction)) + .unwrap_or_default() + } +} + +#[derive(Copy, Clone, Debug, Default, Deserialize, Serialize)] +#[serde(default)] +struct ReactionBehaviorConfig { + issue: ProhibitedReactions, + comment: ProhibitedReactions, +} + +#[derive(Copy, Clone, Debug, Default, Deserialize, Serialize)] +#[serde(default)] +struct ProhibitedReactions { + up_vote: bool, + down_vote: bool, + laugh: bool, + hooray: bool, + confused: bool, + heart: bool, } -#[derive(Debug, Deserialize)] -pub struct FcpBehavior { +impl ProhibitedReactions { + fn is_reaction_prohibited(&self, reaction: Reaction) -> bool { + use self::Reaction::*; + match reaction { + Unknown => false, + Upvote => self.up_vote, + Downvote => self.down_vote, + Laugh => self.laugh, + Hooray => self.hooray, + Confused => self.confused, + Heart => self.heart, + } + } +} + +#[derive(Debug, Deserialize, Serialize)] +struct FcpBehavior { #[serde(default)] close: bool, #[serde(default)] postpone: bool, } -#[derive(Debug, Deserialize)] +#[derive(Debug, Deserialize, Serialize)] pub struct Team { // FIXME(2018-05-16): // The two following first fields are not used anymore. @@ -71,7 +125,7 @@ impl Team { } } -#[derive(Debug, Eq, Hash, Ord, PartialEq, PartialOrd, Deserialize)] +#[derive(Debug, Eq, Hash, Ord, PartialEq, PartialOrd, Deserialize, Serialize)] #[serde(transparent)] pub struct TeamLabel(pub String); @@ -132,6 +186,16 @@ mod test { #[test] fn setup_parser_correct() { let test = r#" +[prohibited_reactions] + +[prohibited_reactions."foo-org/bar".issue] +down_vote = true +confused = true + +[prohibited_reactions."foo-org/bar".comment] +down_vote = true +confused = false + [fcp_behaviors] [fcp_behaviors."rust-lang/alpha"] @@ -208,6 +272,15 @@ members = [ assert!(!cfg.should_ffcp_auto_postpone("wibble/epsilon")); assert!(!cfg.should_ffcp_auto_close("random")); assert!(!cfg.should_ffcp_auto_postpone("random")); + + // Reaction behavior correct: + assert!(cfg.is_issue_reaction_probihited("foo-org/bar", Reaction::Downvote)); + assert!(cfg.is_issue_reaction_probihited("foo-org/bar", Reaction::Confused)); + assert!(!cfg.is_issue_reaction_probihited("foo-org/bar", Reaction::Heart)); + assert!(cfg.is_comment_reaction_probihited("foo-org/bar", Reaction::Downvote)); + assert!(!cfg.is_comment_reaction_probihited("foo-org/bar", Reaction::Confused)); + assert!(!cfg.is_comment_reaction_probihited("foo-org/bar", Reaction::Laugh)); + assert!(!cfg.is_issue_reaction_probihited("random", Reaction::Upvote)); } #[test] From adec1895e38e0bce1254b5248d54cd776d74b6d1 Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Wed, 23 May 2018 09:33:10 +0200 Subject: [PATCH 07/11] add arrayvec as dependency. --- Cargo.lock | 1 + Cargo.toml | 1 + src/main.rs | 1 + 3 files changed, 3 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index 0def97ae..a6797a44 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -702,6 +702,7 @@ dependencies = [ name = "rfcbot-rs" version = "0.1.0" dependencies = [ + "arrayvec 0.4.7 (registry+https://github.com/rust-lang/crates.io-index)", "chrono 0.4.2 (registry+https://github.com/rust-lang/crates.io-index)", "diesel 0.16.0 (registry+https://github.com/rust-lang/crates.io-index)", "diesel_codegen 0.16.0 (registry+https://github.com/rust-lang/crates.io-index)", diff --git a/Cargo.toml b/Cargo.toml index 3b8dcb68..8f0adaed 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -24,6 +24,7 @@ serde_json = "1.0" toml = "0.4" url = "1.4" urlencoded = "0.5" +arrayvec = "0.4.7" [dependencies.chrono] features = ["serde"] diff --git a/src/main.rs b/src/main.rs index f180c3f2..5bd8e55d 100644 --- a/src/main.rs +++ b/src/main.rs @@ -30,6 +30,7 @@ extern crate serde_json; extern crate toml; extern crate url; extern crate urlencoded; +extern crate arrayvec; #[macro_use] mod macros; From 8c8600b0d369f424c15086f651bec30aa149fa45 Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Wed, 23 May 2018 09:37:33 +0200 Subject: [PATCH 08/11] implement reaction nuking logic. --- src/domain/github.rs | 11 --- src/github/client.rs | 199 +++++++++++++++++++++++++++++++------------ src/github/mod.rs | 51 ++++++++++- src/github/models.rs | 72 ++++++++++++++++ src/scraper.rs | 10 ++- src/teams.rs | 52 ++++++----- 6 files changed, 305 insertions(+), 90 deletions(-) diff --git a/src/domain/github.rs b/src/domain/github.rs index 0f699695..567b902a 100644 --- a/src/domain/github.rs +++ b/src/domain/github.rs @@ -149,14 +149,3 @@ pub struct PullRequest { pub changed_files: i32, pub repository: String, } - -#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Debug)] -pub enum Reaction { - Unknown, - Upvote, - Downvote, - Laugh, - Hooray, - Confused, - Heart, -} diff --git a/src/github/client.rs b/src/github/client.rs index 691e8836..192aa0ae 100644 --- a/src/github/client.rs +++ b/src/github/client.rs @@ -18,7 +18,11 @@ use serde_json; use config::CONFIG; use error::{DashError, DashResult}; -use github::models::{CommentFromJson, IssueFromJson, PullRequestFromJson, PullRequestUrls}; +use github::models::{ + CommentFromJson, IssueFromJson, + PullRequestFromJson, PullRequestUrls, + ReactionsIssueFromJson, ReactionsCommentFromJson, ReactionFromJson, Reaction +}; pub const BASE_URL: &'static str = "https://api.github.com"; @@ -26,6 +30,16 @@ 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] } @@ -66,7 +80,7 @@ impl Client { pub fn org_repos(&self, org: &str) -> DashResult> { let url = format!("{}/orgs/{}/repos", BASE_URL, org); - let vals: Vec = try!(self.get_models(&url, None)); + let vals: Vec = self.get_models(&url, None)?; let mut repos = Vec::new(); for v in vals { @@ -84,33 +98,30 @@ impl Client { Ok(repos) } - pub fn issues_since(&self, repo: &str, start: DateTime) -> DashResult> { - - let url = format!("{}/repos/{}/issues", BASE_URL, repo); - let mut params = ParameterMap::new(); - - params.insert("state", "all".to_string()); - params.insert("since", format!("{:?}", start)); - params.insert("state", "all".to_string()); - params.insert("per_page", format!("{}", PER_PAGE)); - params.insert("direction", "asc".to_string()); - - self.get_models(&url, Some(¶ms)) + pub fn issues_since(&self, repo: &str, start: DateTime) + -> DashResult> + { + self.get_models(&format!("{}/repos/{}/issues", BASE_URL, repo), + Some(¶ms! { + "state" => "all".to_string(), + "since" => format!("{:?}", start), + "state" => "all".to_string(), + "per_page" => format!("{}", PER_PAGE), + "direction" => "asc".to_string() + })) } pub fn comments_since(&self, repo: &str, start: DateTime) -> DashResult> { - let url = format!("{}/repos/{}/issues/comments", BASE_URL, repo); - let mut params = ParameterMap::new(); - - params.insert("sort", "created".to_string()); - params.insert("direction", "asc".to_string()); - params.insert("since", format!("{:?}", start)); - params.insert("per_page", format!("{}", PER_PAGE)); - - self.get_models(&url, Some(¶ms)) + self.get_models(&format!("{}/repos/{}/issues/comments", BASE_URL, repo), + Some(¶ms! { + "sort" => "created".to_string(), + "direction" => "asc".to_string(), + "since" => format!("{:?}", start), + "per_page" => format!("{}", PER_PAGE) + })) } fn get_models(&self, @@ -118,7 +129,7 @@ impl Client { params: Option<&ParameterMap>) -> DashResult> { - let mut res = try!(self.get(start_url, params)); + let mut res = self.get(start_url, params)?; let mut models = self.deserialize::>(&mut res)?; while let Some(url) = Self::next_page(&res.headers) { sleep(Duration::from_millis(DELAY)); @@ -128,10 +139,22 @@ impl Client { Ok(models) } - pub fn fetch_pull_request(&self, pr_info: &PullRequestUrls) -> DashResult { - let url = pr_info.get("url"); + fn get_models_preview + (&self, start_url: &str, params: Option<&ParameterMap>) + -> DashResult> { + + let mut res = self.get_preview(start_url, params)?; + let mut models = self.deserialize::>(&mut res)?; + while let Some(url) = Self::next_page(&res.headers) { + sleep(Duration::from_millis(DELAY)); + res = self.get(&url, None)?; + models.extend(self.deserialize::>(&mut res)?); + } + Ok(models) + } - if let Some(url) = url { + pub fn fetch_pull_request(&self, pr_info: &PullRequestUrls) -> DashResult { + if let Some(url) = pr_info.get("url") { let mut res = self.get(url, None)?; self.deserialize(&mut res) } else { @@ -164,11 +187,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 mut obj = BTreeMap::new(); - obj.insert("state", "closed"); - let payload = serde_json::to_string(&obj)?; - + let payload = serde_json::to_string(¶ms!("state" => "closed"))?; let mut res = self.patch(&url, &payload)?; if StatusCode::Ok != res.status { @@ -212,11 +231,7 @@ impl Client { text: &str) -> DashResult { let url = format!("{}/repos/{}/issues/{}/comments", BASE_URL, repo, issue_num); - - let mut obj = BTreeMap::new(); - obj.insert("body", text); - - let payload = serde_json::to_string(&obj)?; + let payload = serde_json::to_string(¶ms!("body" => text))?; // FIXME propagate an error if it's a 404 or other error self.deserialize(&mut self.post(&url, &payload)?) @@ -231,16 +246,77 @@ impl Client { BASE_URL, repo, comment_num); - - let mut obj = BTreeMap::new(); - obj.insert("body", text); - - let payload = serde_json::to_string(&obj)?; + let payload = serde_json::to_string(¶ms!("body" => text))?; // FIXME propagate an error if it's a 404 or other error self.deserialize(&mut self.patch(&url, &payload)?) } + pub fn comments_of_issue(&self, repo: &str, issue_num: i32) + -> DashResult> + { + let url = format!("{}/repos/{}/issues/{}/comments", BASE_URL, repo, issue_num); + let params = params! { + "per_page" => format!("{}", PER_PAGE), + "direction" => "asc".to_string() + }; + Ok(self.get_models_preview(&url, Some(¶ms))?.into_iter()) + } + + pub fn open_issues_with_reactions(&self, repo: &str) + -> DashResult> + { + let url = format!("{}/repos/{}/issues", BASE_URL, repo); + let params = params! { + "state" => "open".to_string(), + "per_page" => format!("{}", PER_PAGE), + "direction" => "asc".to_string() + }; + Ok(self.get_models_preview(&url, Some(¶ms))?.into_iter()) + /* + Ok(issues.into_iter().filter(move |rifj: &ReactionsIssueFromJson| + reactions.iter().any(|&react| rifj.reactions.has_reaction(react)) + )) + */ + } + + pub fn issue_reactions(&self, repo: &str, issue_num: i32, reaction: Reaction) + -> DashResult> + { + self.reactions(reaction, format!( + "{}/repos/{}/issues/{}/reactions", + BASE_URL, repo, issue_num)) + } + + pub fn comment_reactions(&self, repo: &str, comment_id: i32, reaction: Reaction) + -> DashResult> + { + self.reactions(reaction, format!( + "{}/repos/{}/issues/comments/{}/reactions", + BASE_URL, repo, comment_id)) + } + + fn reactions(&self, reaction: Reaction, url: String) + -> DashResult> + { + let params = params! { + "content" => reaction.to_param().into(), + "per_page" => format!("{}", PER_PAGE) + }; + Ok(self.get_models_preview(&url, Some(¶ms))? + .into_iter() + .map(|rfj: ReactionFromJson| rfj.id)) + } + + pub fn delete_reaction(&self, reaction_id: usize) -> DashResult<()> { + let url = format!("{}/reactions/{}", BASE_URL, reaction_id); + let mut res = self.delete_preview(&url)?; + if StatusCode::NoContent != res.status { + throw!(DashError::Misc(Some(read_to_string(&mut res)?))) + } + Ok(()) + } + fn patch(&self, url: &str, payload: &str) -> Result { self.set_headers(self.client.patch(url).body(payload)) .send() @@ -254,11 +330,32 @@ impl Client { self.set_headers(self.client.delete(url)).send() } + fn delete_preview(&self, url: &str) -> Result { + self.set_headers_preview(self.client.delete(url)).send() + } + fn get(&self, url: &str, params: Option<&ParameterMap>) -> Result { - let qp_string = match params { + let qp_string = Self::serialize_qp(params); + let url = format!("{}{}", url, qp_string); + debug!("GETing: {}", &url); + self.set_headers(self.client.get(&url)).send() + } + + fn get_preview(&self, + url: &str, + params: Option<&ParameterMap>) + -> Result { + let qp_string = Self::serialize_qp(params); + let url = format!("{}{}", url, qp_string); + debug!("GETing: {}", &url); + self.set_headers_preview(self.client.get(&url)).send() + } + + fn serialize_qp(params: Option<&ParameterMap>) -> String { + match params { Some(p) => { let mut qp = String::from("?"); for (k, v) in p { @@ -270,19 +367,11 @@ impl Client { qp } None => "".to_string(), - }; - - let url = format!("{}{}", url, qp_string); - - debug!("GETing: {}", &url); - - self.set_headers(self.client.get(&url)).send() + } } fn deserialize(&self, res: &mut Response) -> DashResult { - let mut buf = String::new(); - res.read_to_string(&mut buf)?; - + let buf = read_to_string(res)?; match serde_json::from_str(&buf) { Ok(m) => Ok(m), Err(why) => { @@ -292,6 +381,10 @@ impl Client { } } + fn set_headers_preview<'a>(&self, req: RequestBuilder<'a>) -> RequestBuilder<'a> { + req.header(Accept("application/vnd.github.squirrel-girl-preview+json".to_string())) + } + fn set_headers<'a>(&self, req: RequestBuilder<'a>) -> RequestBuilder<'a> { req.header(Authorization(format!("token {}", &self.token))) .header(UserAgent(self.ua.clone())) diff --git a/src/github/mod.rs b/src/github/mod.rs index 239de9ac..ad64732c 100644 --- a/src/github/mod.rs +++ b/src/github/mod.rs @@ -16,6 +16,7 @@ use DB_POOL; use domain::github::*; use domain::schema::*; use error::DashResult; +use teams::SETUP; use self::client::Client; use self::models::{CommentFromJson, IssueFromJson, PullRequestFromJson}; @@ -61,8 +62,8 @@ pub fn record_successful_update(ingest_start: NaiveDateTime) -> DashResult<()> { pub fn ingest_since(repo: &str, start: DateTime) -> DashResult<()> { info!("fetching all {} issues and comments since {}", repo, start); - let issues = try!(GH.issues_since(repo, start)); - let mut comments = try!(GH.comments_since(repo, start)); + let issues = GH.issues_since(repo, start)?; + let mut comments = GH.comments_since(repo, start)?; // make sure we process the new comments in creation order comments.sort_by_key(|c| c.created_at); @@ -192,6 +193,52 @@ pub fn handle_user(conn: &PgConnection, user: &GitHubUser) -> DashResult<()> { Ok(()) } +pub fn nuke_reactions(repo: &str) -> DashResult<()> { + let issue_reactions = SETUP.prohibited_issue_reactions(repo); + let comment_reactions = SETUP.prohibited_comment_reactions(repo); + + if issue_reactions.is_empty() && comment_reactions.is_empty() { + // All reactions are permitted, bail early to avoid work. + return Ok(()); + } + + // Fetch all issues with some forbidden reaction: + let issues = GH.open_issues_with_reactions(repo)?.filter(|rifj| + issue_reactions.iter().any(|&react| rifj.reactions.has_reaction(react)) + ); + + for issue in issues { + // Remove all forbidden reactions for this issue: + for &reaction in issue_reactions.iter() { + for to_delete in GH.issue_reactions(repo, issue.number, reaction)? { + GH.delete_reaction(to_delete)?; + } + } + + // Ensure we forbid some reactions and that there are comments: + if comment_reactions.is_empty() || issue.comments == 0 { + continue; + } + + // Fetch all comments with some forbidden reaction: + let comments = GH.comments_of_issue(repo, issue.number)?.filter(|rifj| + comment_reactions.iter() + .any(|&react| rifj.reactions.has_reaction(react)) + ); + + for comment in comments { + // Remove all forbidden reactions for this comment: + for &reaction in comment_reactions.iter() { + for to_delete in GH.comment_reactions(repo, comment.id, reaction)? { + GH.delete_reaction(to_delete)?; + } + } + } + } + + Ok(()) +} + #[cfg(test)] mod tests { use super::*; diff --git a/src/github/models.rs b/src/github/models.rs index 045a0d30..74b212c7 100644 --- a/src/github/models.rs +++ b/src/github/models.rs @@ -190,3 +190,75 @@ impl PullRequestFromJson { } } } + +#[derive(Debug, Deserialize)] +pub struct ReactionsIssueFromJson { + pub number: i32, + pub comments: i32, + pub reactions: ReactionSummary, +} + +#[derive(Debug, Deserialize)] +pub struct ReactionsCommentFromJson { + pub id: i32, + pub reactions: ReactionSummary, +} + +#[derive(Debug, Deserialize)] +pub struct ReactionSummary { + #[serde(rename = "+1")] + pub up_vote: u32, + #[serde(rename = "-1")] + pub down_vote: u32, + pub laugh: u32, + pub hooray: u32, + pub confused: u32, + pub heart: u32, +} + +impl ReactionSummary { + pub fn has_reaction(&self, reaction: Reaction) -> bool { + self.reaction_count(reaction) > 0 + } + + pub fn reaction_count(&self, reaction: Reaction) -> u32 { + use self::Reaction::*; + match reaction { + Upvote => self.up_vote, + Downvote => self.down_vote, + Laugh => self.laugh, + Hooray => self.hooray, + Confused => self.confused, + Heart => self.heart, + } + } +} + +#[derive(Debug, Deserialize)] +pub struct ReactionFromJson { + pub id: usize, +} + +#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Debug)] +pub enum Reaction { + Upvote, + Downvote, + Laugh, + Hooray, + Confused, + Heart, +} + +impl Reaction { + pub fn to_param(&self) -> &str { + use self::Reaction::*; + match *self { + Upvote => "+1", + Downvote => "-1", + Laugh => "laugh", + Hooray => "hooray", + Confused => "confused", + Heart => "heart", + } + } +} diff --git a/src/scraper.rs b/src/scraper.rs index 12be988b..2bd6da6a 100644 --- a/src/scraper.rs +++ b/src/scraper.rs @@ -35,13 +35,21 @@ pub fn scrape_github(since: DateTime) { info!("Scraping github activity since {:?}", since); let start_time = Utc::now().naive_utc(); - for repo in repos { + for repo in repos.iter() { match github::ingest_since(&repo, since) { Ok(_) => info!("Scraped {} github successfully", repo), Err(why) => error!("Unable to scrape github {}: {:?}", repo, why), } } + info!("Nuking reactions at github since {:?}", since); + for repo in repos.iter() { + match github::nuke_reactions(&repo) { + Ok(_) => info!("Nuked reactions at {} successfully", repo), + Err(why) => error!("Unable to nuke reactions {}: {:?}", repo, why), + } + } + ok_or!(github::record_successful_update(start_time), why => error!("Problem recording successful update: {:?}", why)); } diff --git a/src/teams.rs b/src/teams.rs index 7ba2030a..8f97d1e9 100644 --- a/src/teams.rs +++ b/src/teams.rs @@ -4,9 +4,11 @@ use std::collections::BTreeMap; use diesel::prelude::*; use toml; +use arrayvec::ArrayVec; use super::DB_POOL; -use domain::github::{Reaction, GitHubUser}; +use domain::github::{GitHubUser}; +use github::models::Reaction; use error::*; //============================================================================== @@ -45,23 +47,17 @@ impl RfcbotConfig { self.fcp_behaviors.get(repo).map(|fcp| fcp.postpone).unwrap_or_default() } - /// Is the reaction permitted on issues / PRs of this repository? - pub fn is_issue_reaction_probihited(&self, repo: &str, reaction: Reaction) - -> bool - { - self.prohibited_reactions - .get(repo) - .map(|rb| rb.issue.is_reaction_prohibited(reaction)) + /// Returns all the prohibited reactions on issues for this repo. + pub fn prohibited_issue_reactions(&self, repo: &str) -> ReactionVec { + self.prohibited_reactions.get(repo) + .map(|rb| rb.issue.prohibited_reactions()) .unwrap_or_default() } - /// Is the reaction permitted on comments of issues / PRs of this repository? - pub fn is_comment_reaction_probihited(&self, repo: &str, reaction: Reaction) - -> bool - { - self.prohibited_reactions - .get(repo) - .map(|rb| rb.comment.is_reaction_prohibited(reaction)) + /// Returns all the prohibited reactions on comments for this repo. + pub fn prohibited_comment_reactions(&self, repo: &str) -> ReactionVec { + self.prohibited_reactions.get(repo) + .map(|rb| rb.comment.prohibited_reactions()) .unwrap_or_default() } } @@ -84,11 +80,20 @@ struct ProhibitedReactions { heart: bool, } +type ReactionVec = ArrayVec<[Reaction; 6]>; + impl ProhibitedReactions { + fn prohibited_reactions(&self) -> ReactionVec { + use self::Reaction::*; + [Upvote, Downvote, Laugh, Hooray, Confused, Heart].iter() + .filter(move |&&reaction| self.is_reaction_prohibited(reaction)) + .cloned() + .collect() + } + fn is_reaction_prohibited(&self, reaction: Reaction) -> bool { use self::Reaction::*; match reaction { - Unknown => false, Upvote => self.up_vote, Downvote => self.down_vote, Laugh => self.laugh, @@ -274,13 +279,14 @@ members = [ assert!(!cfg.should_ffcp_auto_postpone("random")); // Reaction behavior correct: - assert!(cfg.is_issue_reaction_probihited("foo-org/bar", Reaction::Downvote)); - assert!(cfg.is_issue_reaction_probihited("foo-org/bar", Reaction::Confused)); - assert!(!cfg.is_issue_reaction_probihited("foo-org/bar", Reaction::Heart)); - assert!(cfg.is_comment_reaction_probihited("foo-org/bar", Reaction::Downvote)); - assert!(!cfg.is_comment_reaction_probihited("foo-org/bar", Reaction::Confused)); - assert!(!cfg.is_comment_reaction_probihited("foo-org/bar", Reaction::Laugh)); - assert!(!cfg.is_issue_reaction_probihited("random", Reaction::Upvote)); + let foo_issue_nono = cfg.prohibited_issue_reactions("foo-org/bar") + .into_iter().collect::>(); + assert_eq!(foo_issue_nono, &[Reaction::Downvote, Reaction::Confused]); + let foo_comment_nono = cfg.prohibited_comment_reactions("foo-org/bar") + .into_iter().collect::>(); + assert_eq!(foo_comment_nono, &[Reaction::Downvote]); + assert!(cfg.prohibited_issue_reactions("random").is_empty()); + assert!(cfg.prohibited_comment_reactions("random").is_empty()); } #[test] From 6fb5b8b202cdae07dd035bb563a061fe8673eac0 Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Wed, 23 May 2018 09:37:45 +0200 Subject: [PATCH 09/11] document reaction nuking in changelog. --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 188e1543..7222abe8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,3 +8,7 @@ rfcbot behaves so that the people who develop rfcbot or interact with it can understand the bot better. ## Changes + ++ The bot will now optionally (per configuration in `rfcbot.toml`) remove + unwanted reactions from RFC (and PR..) readers. + These include 👎, 😕 on the `rust-lang/rfcs` and `rust-lang/rust` repositories. From 3f6dc3df5e478a4e78421b6f37c68b3d6efa22b8 Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Wed, 23 May 2018 09:43:38 +0200 Subject: [PATCH 10/11] teams.rs: get rid of Serialize impls --- src/teams.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/teams.rs b/src/teams.rs index 8f97d1e9..13bfdcc7 100644 --- a/src/teams.rs +++ b/src/teams.rs @@ -19,7 +19,7 @@ lazy_static! { pub static ref SETUP: RfcbotConfig = read_rfcbot_cfg_validated(); } -#[derive(Debug, Deserialize, Serialize)] +#[derive(Debug, Deserialize)] pub struct RfcbotConfig { prohibited_reactions: BTreeMap, fcp_behaviors: BTreeMap, @@ -62,14 +62,14 @@ impl RfcbotConfig { } } -#[derive(Copy, Clone, Debug, Default, Deserialize, Serialize)] +#[derive(Copy, Clone, Debug, Default, Deserialize)] #[serde(default)] struct ReactionBehaviorConfig { issue: ProhibitedReactions, comment: ProhibitedReactions, } -#[derive(Copy, Clone, Debug, Default, Deserialize, Serialize)] +#[derive(Copy, Clone, Debug, Default, Deserialize)] #[serde(default)] struct ProhibitedReactions { up_vote: bool, @@ -104,7 +104,7 @@ impl ProhibitedReactions { } } -#[derive(Debug, Deserialize, Serialize)] +#[derive(Debug, Deserialize)] struct FcpBehavior { #[serde(default)] close: bool, @@ -112,7 +112,7 @@ struct FcpBehavior { postpone: bool, } -#[derive(Debug, Deserialize, Serialize)] +#[derive(Debug, Deserialize)] pub struct Team { // FIXME(2018-05-16): // The two following first fields are not used anymore. @@ -130,7 +130,7 @@ impl Team { } } -#[derive(Debug, Eq, Hash, Ord, PartialEq, PartialOrd, Deserialize, Serialize)] +#[derive(Debug, Eq, Hash, Ord, PartialEq, PartialOrd, Deserialize)] #[serde(transparent)] pub struct TeamLabel(pub String); From 62c7de86e0600228a4d11e28e86a8edbfc46d8a9 Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Wed, 23 May 2018 10:16:20 +0200 Subject: [PATCH 11/11] cleanup leftover junk comments --- src/github/client.rs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/github/client.rs b/src/github/client.rs index 192aa0ae..e57eecff 100644 --- a/src/github/client.rs +++ b/src/github/client.rs @@ -273,11 +273,6 @@ impl Client { "direction" => "asc".to_string() }; Ok(self.get_models_preview(&url, Some(¶ms))?.into_iter()) - /* - Ok(issues.into_iter().filter(move |rifj: &ReactionsIssueFromJson| - reactions.iter().any(|&react| rifj.reactions.has_reaction(react)) - )) - */ } pub fn issue_reactions(&self, repo: &str, issue_num: i32, reaction: Reaction)