Repository: cocogitto/cocogitto (rust)

Issue:
[FEATURE] Allow running `cog check` on a commit range
**Is your feature request related to a problem? Please describe.**

As of now, the `cog check` command can be run either on all commits, or only on commits starting from the latest tag. There is no option to run `cog check` on a commit range (say, the commits associated with a PR).

**Describe the solution you'd like**

It would be preferable if we could have an option to run the `cog check` command on a commit range as well. I did go through the code, and noticed that the necessary utilities to parse a revision range from a string are already available, so this should be a simple addition. I'd be happy to open a PR if required.

**Describe alternatives you've considered**

For the exact problem I'm trying to solve (checking commits in a PR for compliance to the conventional commit specification), the cocogitto bot is a solution. However, we also have an additional requirement that the PR title must also comply to the conventional commit specification since we do squash merging. To solve for this, I was hoping to run `cog verify` against the PR title, and `cog check` against the commit range in a single workflow. I'm open to any other solutions you may have.

**Additional context**

N/A


Pull request:
feat(check): allow running check on commit range
Closes #284.

The changes included in this PR allows accepting the commit range as an optional CLI argument. However, `clap` would raise an error if both `--from-latest-tag` and commit range are specified. This PR also includes some unit tests, let me know if any more tests are needed.

A problem that I see with the current implementation is that parsing a commit range from a string panics if the string is of an invalid format (check the `From<&str>` implementation of `RevspecPattern`). This would be a bad experience for the users. We should consider implementing `FromStr` or `TryFrom` instead, and raise an error instead of panicking. Depending on what's preferable, I can take this up on a separate PR.

Patch:
diff --git a/src/bin/cog/main.rs b/src/bin/cog/main.rs
index d1c5f96..16ac157 100644
--- a/src/bin/cog/main.rs
+++ b/src/bin/cog/main.rs
@@ -125,12 +125,16 @@ enum Command {
     /// Verify all commit messages against the conventional commit specification
     Check {
         /// Check commit history, starting from the latest tag to HEAD
-        #[arg(short = 'l', long)]
+        #[arg(short = 'l', long, group = "commit_range")]
         from_latest_tag: bool,
 
         /// Ignore merge commits messages
         #[arg(short, long)]
         ignore_merge_commits: bool,
+
+        /// Check commits in the specified range
+        #[arg(group = "commit_range")]
+        range: Option<String>,
     },
 
     /// Create a new conventional commit
@@ -423,11 +427,12 @@ fn main() -> Result<()> {
         Command::Check {
             from_latest_tag,
             ignore_merge_commits,
+            range,
         } => {
             let cocogitto = CocoGitto::get()?;
             let from_latest_tag = from_latest_tag || SETTINGS.from_latest_tag;
             let ignore_merge_commits = ignore_merge_commits || SETTINGS.ignore_merge_commits;
-            cocogitto.check(from_latest_tag, ignore_merge_commits)?;
+            cocogitto.check(from_latest_tag, ignore_merge_commits, range)?;
         }
         Command::Edit { from_latest_tag } => {
             let cocogitto = CocoGitto::get()?;
diff --git a/src/command/check.rs b/src/command/check.rs
index ead91ac..9fa30d5 100644
--- a/src/command/check.rs
+++ b/src/command/check.rs
@@ -8,8 +8,16 @@ use colored::*;
 use log::info;
 
 impl CocoGitto {
-    pub fn check(&self, check_from_latest_tag: bool, ignore_merge_commits: bool) -> Result<()> {
-        let commit_range = if check_from_latest_tag {
+    pub fn check(
+        &self,
+        check_from_latest_tag: bool,
+        ignore_merge_commits: bool,
+        range: Option<String>,
+    ) -> Result<()> {
+        let commit_range = if let Some(range) = range {
+            self.repository
+                .get_commit_range(&RevspecPattern::from(range.as_str()))?
+        } else if check_from_latest_tag {
             self.repository
                 .get_commit_range(&RevspecPattern::default())?
         } else {


Test patch:
diff --git a/tests/cog_tests/check.rs b/tests/cog_tests/check.rs
index aad069c..8f8f2e4 100644
--- a/tests/cog_tests/check.rs
+++ b/tests/cog_tests/check.rs
@@ -83,3 +83,63 @@ fn cog_check_from_latest_tag_failure() -> Result<()> {
         .stderr(predicate::str::contains("Found 1 non compliant commits"));
     Ok(())
 }
+
+#[sealed_test]
+fn cog_check_commit_range_ok() -> Result<()> {
+    // Arrange
+    git_init()?;
+    let range_start = git_commit("chore: init")?;
+    git_commit("feat: feature")?;
+    let range_end = git_commit("fix: bug fix")?;
+    let range = format!("{range_start}..{range_end}");
+
+    // Act
+    Command::cargo_bin("cog")?
+        .arg("check")
+        .arg(range)
+        // Assert
+        .assert()
+        .success()
+        .stderr(predicate::str::contains("No errored commits"));
+    Ok(())
+}
+
+#[sealed_test]
+fn cog_check_commit_range_failure() -> Result<()> {
+    // Arrange
+    git_init()?;
+    let range_start = git_commit("chore: init")?;
+    git_commit("toto: errored commit")?;
+    git_commit("feat: feature")?;
+    git_commit("fix: bug fix")?;
+    let range_end = git_commit("toto: africa")?;
+    let range = format!("{range_start}..{range_end}");
+
+    // Act
+    Command::cargo_bin("cog")?
+        .arg("check")
+        .arg(range)
+        // Assert
+        .assert()
+        .failure()
+        .stderr(predicate::str::contains("Found 2 non compliant commits"));
+    Ok(())
+}
+
+#[sealed_test]
+fn cog_check_from_latest_tag_and_commit_range_failure() -> Result<()> {
+    // Arrange
+
+    // Act
+    Command::cargo_bin("cog")?
+        .arg("check")
+        .arg("--from-latest-tag")
+        .arg("abcdef..fedcba")
+        // Assert
+        .assert()
+        .failure()
+        .stderr(predicate::str::contains(
+            "the argument '--from-latest-tag' cannot be used with '[RANGE]'",
+        ));
+    Ok(())
+}
diff --git a/tests/lib_tests/cocogitto.rs b/tests/lib_tests/cocogitto.rs
index 897db57..2378925 100644
--- a/tests/lib_tests/cocogitto.rs
+++ b/tests/lib_tests/cocogitto.rs
@@ -42,7 +42,7 @@ fn check_commit_history_ok() -> Result<()> {
     let cocogitto = CocoGitto::get()?;
 
     // Act
-    let check = cocogitto.check(false, false);
+    let check = cocogitto.check(false, false, None);
 
     // Assert
     assert_that!(check).is_ok();
@@ -58,7 +58,7 @@ fn check_commit_history_err_with_merge_commit() -> Result<()> {
     let cocogitto = CocoGitto::get()?;
 
     // Act
-    let check = cocogitto.check(false, false);
+    let check = cocogitto.check(false, false, None);
 
     // Assert
     assert_that!(check).is_err();
@@ -81,7 +81,7 @@ fn check_commit_history_ok_with_merge_commit_ignored() -> Result<()> {
     let cocogitto = CocoGitto::get()?;
 
     // Act
-    let check = cocogitto.check(false, true);
+    let check = cocogitto.check(false, true, None);
 
     // Assert
     assert_that!(check).is_ok();
@@ -98,7 +98,7 @@ fn check_commit_history_err() -> Result<()> {
     let cocogitto = CocoGitto::get()?;
 
     // Act
-    let check = cocogitto.check(false, false);
+    let check = cocogitto.check(false, false, None);
 
     // Assert
     assert_that!(check).is_err();
@@ -117,7 +117,7 @@ fn check_commit_ok_from_latest_tag() -> Result<()> {
     let cocogitto = CocoGitto::get()?;
 
     // Act
-    let check = cocogitto.check(true, false);
+    let check = cocogitto.check(true, false, None);
 
     // Assert
     assert_that!(check).is_ok();
@@ -135,7 +135,7 @@ fn check_commit_err_from_latest_tag() -> Result<()> {
     let cocogitto = CocoGitto::get()?;
 
     // Act
-    let check = cocogitto.check(true, false);
+    let check = cocogitto.check(true, false, None);
 
     // Assert
     assert_that!(check).is_err();
@@ -152,8 +152,84 @@ fn long_commit_summary_does_not_panic() -> Result<()> {
     git_add("Hello", "file")?;
     cocogitto.conventional_commit("feat", None, message, None, None, false, false)?;
 
-    let check = cocogitto.check(false, false);
+    let check = cocogitto.check(false, false, None);
 
     assert_that!(check.is_ok());
     Ok(())
 }
+
+#[sealed_test]
+fn check_commit_ok_commit_range() -> Result<()> {
+    // Arrange
+    git_init()?;
+    let range_start = git_commit("feat: a valid commit")?;
+    let range_end = git_commit("chore(test): another valid commit")?;
+    let range = format!("{range_start}..{range_end}");
+    let cocogitto = CocoGitto::get()?;
+
+    // Act
+    let check = cocogitto.check(true, false, Some(range));
+
+    // Assert
+    assert_that!(check).is_ok();
+    Ok(())
+}
+
+#[sealed_test]
+fn check_commit_err_commit_range() -> Result<()> {
+    // Arrange
+    git_init_and_set_current_path("commit_history_err")?;
+    create_empty_config()?;
+    let range_start = git_commit("feat: a valid commit")?;
+    let range_end = git_commit("errored commit")?;
+    let range = format!("{range_start}..{range_end}");
+    let cocogitto = CocoGitto::get()?;
+
+    // Act
+    let check = cocogitto.check(true, false, Some(range));
+
+    // Assert
+    assert_that!(check).is_err();
+    Ok(())
+}
+
+#[sealed_test]
+fn check_commit_range_err_with_merge_commit() -> Result<()> {
+    // Arrange
+    git_init()?;
+    let range_start = git_commit("feat: a valid commit")?;
+    let range_end = git_commit("Merge feature one into main")?;
+    let range = format!("{range_start}..{range_end}");
+    let cocogitto = CocoGitto::get()?;
+
+    // Act
+    let check = cocogitto.check(false, false, Some(range));
+
+    // Assert
+    assert_that!(check).is_err();
+    Ok(())
+}
+
+#[sealed_test]
+fn check_commit_range_ok_with_merge_commit_ignored() -> Result<()> {
+    // Arrange
+    git_init()?;
+    let range_start = git_commit("feat: a valid commit")?;
+    run_cmd!(git checkout -b branch;)?;
+    git_commit("feat: commit on another branch")?;
+    run_cmd!(
+        git checkout -;
+        git merge branch --no-ff;
+        git --no-pager log;
+    )?;
+    let range_end = git_commit("chore(test): another valid commit")?;
+    let range = format!("{range_start}..{range_end}");
+    let cocogitto = CocoGitto::get()?;
+
+    // Act
+    let check = cocogitto.check(false, true, Some(range));
+
+    // Assert
+    assert_that!(check).is_ok();
+    Ok(())
+}