Merge pull request #1922 from Urgau/extract-out-of-assign

Extract non-default-branch and modifies-submodules warnings out of the `assign` handler

Author: Kobzol

src/handlers.rs

@@ -27,6 +27,7 @@ impl fmt::Display for HandlerError {
 mod assign;
 mod autolabel;
 mod bot_pull_requests;
+mod check_commits;
 mod close;
 pub mod docs_update;
 mod github_releases;
@@ -71,6 +72,16 @@ pub async fn handle(ctx: &Context, event: &Event) -> Vec<HandlerError> {
         handle_command(ctx, event, &config, body, &mut errors).await;
     }
 
+    if let Ok(config) = &config {
+        if let Err(e) = check_commits::handle(ctx, event, &config).await {
+            log::error!(
+                "failed to process event {:?} with `check_commits` handler: {:?}",
+                event,
+                e
+            );
+        }
+    }
+
     if let Err(e) = project_goals::handle(ctx, event).await {
         log::error!(
             "failed to process event {:?} with `project_goals` handler: {:?}",

src/handlers/assign.rs

@@ -21,7 +21,7 @@
 //! the PR modifies.
 
 use crate::{
-    config::{AssignConfig, WarnNonDefaultBranchException},
+    config::AssignConfig,
     github::{self, Event, FileDiff, Issue, IssuesAction, Selection},
     handlers::{Context, GithubClient, IssuesEvent},
     interactions::EditIssueBody,
@@ -74,18 +74,6 @@ const ON_VACATION_WARNING: &str = "{username} is on vacation.
 
 Please choose another assignee.";
 
-const NON_DEFAULT_BRANCH: &str =
-    "Pull requests are usually filed against the {default} branch for this repo, \
-     but this one is against {target}. \
-     Please double check that you specified the right target!";
-
-const NON_DEFAULT_BRANCH_EXCEPTION: &str =
-    "Pull requests targetting the {default} branch are usually filed against the {default} \
-     branch, but this one is against {target}. \
-     Please double check that you specified the right target!";
-
-const SUBMODULE_WARNING_MSG: &str = "These commits modify **submodules**.";
-
 pub const SELF_ASSIGN_HAS_NO_CAPACITY: &str = "
 You have insufficient capacity to be assigned the pull request at this time. PR assignment has been reverted.
 
@@ -218,20 +206,6 @@ pub(super) async fn handle_input(
         }
     }
 
-    // Compute some warning messages to post to new PRs.
-    let mut warnings = Vec::new();
-    if let Some(exceptions) = config.warn_non_default_branch.enabled_and_exceptions() {
-        warnings.extend(non_default_branch(exceptions, event));
-    }
-    warnings.extend(modifies_submodule(diff));
-    if !warnings.is_empty() {
-        let warnings: Vec<_> = warnings
-            .iter()
-            .map(|warning| format!("* {warning}"))
-            .collect();
-        let warning = format!(":warning: **Warning** :warning:\n\n{}", warnings.join("\n"));
-        event.issue.post_comment(&ctx.github, &warning).await?;
-    };
     Ok(())
 }
 
@@ -250,40 +224,6 @@ fn is_self_assign(assignee: &str, pr_author: &str) -> bool {
     assignee.to_lowercase() == pr_author.to_lowercase()
 }
 
-/// Returns a message if the PR is opened against the non-default branch (or the exception branch
-/// if it's an exception).
-fn non_default_branch(
-    exceptions: &[WarnNonDefaultBranchException],
-    event: &IssuesEvent,
-) -> Option<String> {
-    let target_branch = &event.issue.base.as_ref().unwrap().git_ref;
-    let (default_branch, warn_msg) = exceptions
-        .iter()
-        .find(|e| event.issue.title.contains(&e.title))
-        .map_or_else(
-            || (&event.repository.default_branch, NON_DEFAULT_BRANCH),
-            |e| (&e.branch, NON_DEFAULT_BRANCH_EXCEPTION),
-        );
-    if target_branch == default_branch {
-        return None;
-    }
-    Some(
-        warn_msg
-            .replace("{default}", default_branch)
-            .replace("{target}", target_branch),
-    )
-}
-
-/// Returns a message if the PR modifies a git submodule.
-fn modifies_submodule(diff: &[FileDiff]) -> Option<String> {
-    let re = regex::Regex::new(r"\+Subproject\scommit\s").unwrap();
-    if diff.iter().any(|fd| re.is_match(&fd.diff)) {
-        Some(SUBMODULE_WARNING_MSG.to_string())
-    } else {
-        None
-    }
-}
-
 /// Sets the assignee of a PR, alerting any errors.
 async fn set_assignee(issue: &Issue, github: &GithubClient, username: &str) {
     // Don't re-assign if already assigned, e.g. on comment edit

src/handlers/check_commits.rs

@@ -0,0 +1,53 @@
+use anyhow::bail;
+
+use super::Context;
+use crate::{
+    config::Config,
+    github::{Event, IssuesAction},
+};
+
+mod modified_submodule;
+mod non_default_branch;
+
+pub(super) async fn handle(ctx: &Context, event: &Event, config: &Config) -> anyhow::Result<()> {
+    let Event::Issue(event) = event else {
+        return Ok(());
+    };
+
+    if !matches!(event.action, IssuesAction::Opened) || !event.issue.is_pr() {
+        return Ok(());
+    }
+
+    let Some(diff) = event.issue.diff(&ctx.github).await? else {
+        bail!(
+            "expected issue {} to be a PR, but the diff could not be determined",
+            event.issue.number
+        )
+    };
+
+    let mut warnings = Vec::new();
+
+    if let Some(assign_config) = &config.assign {
+        // For legacy reasons the non-default-branch and modifies-submodule warnings
+        // are behind the `[assign]` config.
+
+        if let Some(exceptions) = assign_config
+            .warn_non_default_branch
+            .enabled_and_exceptions()
+        {
+            warnings.extend(non_default_branch::non_default_branch(exceptions, event));
+        }
+        warnings.extend(modified_submodule::modifies_submodule(diff));
+    }
+
+    if !warnings.is_empty() {
+        let warnings: Vec<_> = warnings
+            .iter()
+            .map(|warning| format!("* {warning}"))
+            .collect();
+        let warning = format!(":warning: **Warning** :warning:\n\n{}", warnings.join("\n"));
+        event.issue.post_comment(&ctx.github, &warning).await?;
+    };
+
+    Ok(())
+}

src/handlers/check_commits/modified_submodule.rs

@@ -0,0 +1,13 @@
+use crate::github::FileDiff;
+
+const SUBMODULE_WARNING_MSG: &str = "These commits modify **submodules**.";
+
+/// Returns a message if the PR modifies a git submodule.
+pub(super) fn modifies_submodule(diff: &[FileDiff]) -> Option<String> {
+    let re = regex::Regex::new(r"\+Subproject\scommit\s").unwrap();
+    if diff.iter().any(|fd| re.is_match(&fd.diff)) {
+        Some(SUBMODULE_WARNING_MSG.to_string())
+    } else {
+        None
+    }
+}

src/handlers/check_commits/non_default_branch.rs

@@ -0,0 +1,44 @@
+use crate::{config::WarnNonDefaultBranchException, github::IssuesEvent};
+
+/// Returns a message if the PR is opened against the non-default branch (or the
+/// exception branch if it's an exception).
+pub(super) fn non_default_branch(
+    exceptions: &[WarnNonDefaultBranchException],
+    event: &IssuesEvent,
+) -> Option<String> {
+    let target_branch = &event.issue.base.as_ref().unwrap().git_ref;
+
+    if let Some(exception) = exceptions
+        .iter()
+        .find(|e| event.issue.title.contains(&e.title))
+    {
+        if &exception.branch != target_branch {
+            return Some(not_default_exception_branch_warn(
+                &exception.branch,
+                target_branch,
+            ));
+        }
+    } else if &event.repository.default_branch != target_branch {
+        return Some(not_default_branch_warn(
+            &event.repository.default_branch,
+            target_branch,
+        ));
+    }
+    None
+}
+
+fn not_default_branch_warn(default: &str, target: &str) -> String {
+    format!(
+        "Pull requests are usually filed against the {default} branch for this repo, \
+         but this one is against {target}. \
+         Please double check that you specified the right target!"
+    )
+}
+
+fn not_default_exception_branch_warn(default: &str, target: &str) -> String {
+    format!(
+        "Pull requests targetting the {default} branch are usually filed against the {default} \
+         branch, but this one is against {target}. \
+         Please double check that you specified the right target!"
+    )
+}