Repository: cocogitto/cocogitto
Pull request #288 (2023-06-08 17:45:26)

feat: Add configurable changelog omission for custom commit types
PR enables configurable changelog omission by commit type, as requested in #214 

> **NOTE:** _implemenation is fairly naive, but is a completely functional "first draft" implementation_

cocogitto-bot[bot]: :heavy_check_mark: 5a01106 - Conventional commits check succeeded.
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/cocogitto/cocogitto/pull/288?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cocogitto) Report
> Merging [#288](https://app.codecov.io/gh/cocogitto/cocogitto/pull/288?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cocogitto) (5a01106) into [main](https://app.codecov.io/gh/cocogitto/cocogitto/commit/4a09837244e070ff6168cd247ed5621b41f4264e?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cocogitto) (4a09837) will **decrease** coverage by `0.09%`.
> The diff coverage is `50.00%`.

```diff
@@            Coverage Diff             @@
##             main     #288      +/-   ##
==========================================
- Coverage   85.56%   85.48%   -0.09%     
==========================================
  Files          44       44              
  Lines        6014     6027      +13     
==========================================
+ Hits         5146     5152       +6     
- Misses        868      875       +7     
```


| [Impacted Files](https://app.codecov.io/gh/cocogitto/cocogitto/pull/288?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cocogitto) | Coverage Δ | |
|---|---|---|
| [src/conventional/commit.rs](https://app.codecov.io/gh/cocogitto/cocogitto/pull/288?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cocogitto#diff-c3JjL2NvbnZlbnRpb25hbC9jb21taXQucnM=) | `91.76% <40.00%> (-1.29%)` | :arrow_down: |
| [src/conventional/changelog/release.rs](https://app.codecov.io/gh/cocogitto/cocogitto/pull/288?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cocogitto#diff-c3JjL2NvbnZlbnRpb25hbC9jaGFuZ2Vsb2cvcmVsZWFzZS5ycw==) | `99.62% <75.00%> (-0.19%)` | :arrow_down: |

:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cocogitto)

bdarcus: @the-wondersmith - how would it work in terms of config?
the-wondersmith: @bdarcus If you look at the additions, it's fairly simple. I've added a field to  [`conventional/commit.rs`](https://github.com/cocogitto/cocogitto/blob/5a0110696939d8307086721c3ec17cea0f432f25/src/conventional/commit.rs#L25) called `omit_from_changelog`. The new field defaults to `false` and is marked with `#[serde(default)]` both to ensure that there's no conflict with existing `cog.toml` files and to make it (effectively) optional. Unless explicitly configured otherwise, all commits will appear in generated changelogs - in other words the current behavior is preserved, the new behavior is explicitly opt-in.

[`CommitConfig::new`'s](https://github.com/cocogitto/cocogitto/blob/5a0110696939d8307086721c3ec17cea0f432f25/src/conventional/commit.rs#L29) calling signature requires `changelog_title` to be supplied, meaning it shouldn't (really _can't_, technically) be changed without a considerable amount of forethought. Also, changing that would likely qualify as a breaking change to the internal API. To maintain the general usability though I've included two new methods to `CommitConfig` ([`omit`](https://github.com/cocogitto/cocogitto/blob/5a0110696939d8307086721c3ec17cea0f432f25/src/conventional/commit.rs#L37) and [`include`](https://github.com/cocogitto/cocogitto/blob/5a0110696939d8307086721c3ec17cea0f432f25/src/conventional/commit.rs#L42)) that allow for toggling the new flag on and off, in case that ever becomes necessary in the future.

Looking through the "flow" of things when changelogs are being generated, the least disruptive place I could see to add a check for whether or not a given commit should be omitted that wouldn't _also_ result in cascading changes to various calling signatures / return types, I settled on [`CommitRange::from`](https://github.com/cocogitto/cocogitto/blob/5a0110696939d8307086721c3ec17cea0f432f25/src/conventional/changelog/release.rs#L53) and [implemented the check](https://github.com/cocogitto/cocogitto/blob/5a0110696939d8307086721c3ec17cea0f432f25/src/conventional/changelog/release.rs#L64) such that it simply declines to append any commit that's configured as omitted to the vec of commits being assembled in that method.

That specific choice of "shim" location necessitated some way to check if a `Commit` _should_ be omitted, but I couldn't find any **_direct / explicit_** link between any given `Commit` and its associated `CommitConfig` until I saw how they're linked up in the [`Settings` impl](https://github.com/cocogitto/cocogitto/blob/5a0110696939d8307086721c3ec17cea0f432f25/src/settings/mod.rs#L190). Once I found that, I just sorta... extrapolated from there and added a [`Commit::should_omit`](https://github.com/cocogitto/cocogitto/blob/5a0110696939d8307086721c3ec17cea0f432f25/src/conventional/commit.rs#L103) method that allows a `Commit` to check itself against the global `Settings` and determine whether or not it should be omitted from changelog assembly 🙂.

Putting it all together, I updated the `cog.toml` for another project of mine to define a `wip` commit type that's _not_ included in changelogs:

```toml
[commit_types]
hotfix = { changelog_title = "Hotfixes" }
release = { changelog_title = "Releases" }
wip = { changelog_title = "Work In Progress", omit_from_changelog = true }
```

Then generated changelogs -

---
<img width="800" alt="before" src="https://github.com/cocogitto/cocogitto/assets/61921871/a91f3288-939f-4f9a-a1c0-efb3e5b9bc30">

---
<img width="800" alt="after" src="https://github.com/cocogitto/cocogitto/assets/61921871/ae949908-ade5-4cca-b61f-a5b4b019d11b">


cocogitto-bot[bot]: :heavy_check_mark: 5a01106...8988749 - Conventional commits check succeeded.
the-wondersmith: @oknozor Of course. I pushed fixes for the linting issues, and clippy passes locally as well. `llvm-cov` has a few tests failing on my local machine (which is mildly concerning 🤔), but nothing failed when actions ran the test suite previously, so I'll have to assume that's some weird local quirk.

Please let me know if there's anything else I can do - eager for your review and/or thoughts otherwise 😁.
cocogitto-bot[bot]: :heavy_check_mark: 5a01106...651e920 - Conventional commits check succeeded.
cocogitto-bot[bot]: :heavy_check_mark: 5a01106...36fe2eb - Conventional commits check succeeded.
the-wondersmith: @oknozor [test added](https://github.com/cocogitto/cocogitto/blob/36fe2eb2bff7ad1850ab1348308b39bc3b1fd4fe/tests/cog_tests/changelog.rs#L445).

Also you are 100% correct - tests are failing locally due to the exact situation described by #235 😅
cocogitto-bot[bot]: :heavy_check_mark: 5a01106...159587d - Conventional commits check succeeded.
the-wondersmith: @oknozor Saw that everything was in the green except for coverage of the new `omit` and `include` utility methods. Presumably the coverage doesn't run on nightly, so the attributes aren't applied.

Reverted the addition of `no_coverage` support and added a simple test for the two that ensures that the flag behaves as advertised. _Should_ be green across the board now (whenever everything runs).
oknozor: @the-wondersmith there are still a few pending comments in re review
oknozor: @the-wondersmith look at the "Pending" comments here https://github.com/cocogitto/cocogitto/pull/288/files
the-wondersmith: @oknozor all I've got on my end is:

> <img width="436" alt="no conversations" src="https://github.com/cocogitto/cocogitto/assets/61921871/e20216fe-22f5-45a0-812a-95097a3286de">

I _absolutely_ don't want to presume, but is there any chance you [haven't actually submitted](https://github.com/orgs/community/discussions/10369) the review(s) in question?
cocogitto-bot[bot]: :heavy_check_mark: 5a01106...30207ce - Conventional commits check succeeded.
oknozor: I did forgot to submit the review, oopsy :) 
cocogitto-bot[bot]: :heavy_check_mark: 5a01106...4dea994 - Conventional commits check succeeded.
the-wondersmith: @oknozor all requested changes made and pushed
the-wondersmith: @oknozor We appear to be green across the board 😁

Patch:
diff --git a/src/conventional/changelog/release.rs b/src/conventional/changelog/release.rs
index fbe8ab0..71dc060 100644
--- a/src/conventional/changelog/release.rs
+++ b/src/conventional/changelog/release.rs
@@ -60,7 +60,11 @@ impl<'a> From<CommitRange<'a>> for Release<'a> {
             }
 
             match Commit::from_git_commit(&commit) {
-                Ok(commit) => commits.push(ChangelogCommit::from(commit)),
+                Ok(commit) => {
+                    if !commit.should_omit() {
+                        commits.push(ChangelogCommit::from(commit))
+                    }
+                }
                 Err(err) => {
                     let err = err.to_string().red();
                     warn!("{}", err);
diff --git a/src/conventional/commit.rs b/src/conventional/commit.rs
index acd49a4..4be76fc 100644
--- a/src/conventional/commit.rs
+++ b/src/conventional/commit.rs
@@ -21,12 +21,15 @@ pub struct Commit {
 #[derive(Debug, Deserialize, Serialize, Clone, Eq, PartialEq)]
 pub struct CommitConfig {
     pub changelog_title: String,
+    #[serde(default)]
+    pub omit_from_changelog: bool,
 }
 
 impl CommitConfig {
     pub(crate) fn new(changelog_title: &str) -> Self {
         CommitConfig {
             changelog_title: changelog_title.to_string(),
+            omit_from_changelog: false,
         }
     }
 }
@@ -87,6 +90,13 @@ impl Commit {
         }
     }
 
+    pub(crate) fn should_omit(&self) -> bool {
+        SETTINGS
+            .commit_types()
+            .get(&self.message.commit_type)
+            .map_or(false, |config| config.omit_from_changelog)
+    }
+
     pub fn get_log(&self) -> String {
         let summary = &self.message.summary;
         let message_display = Commit::short_summary_from_str(summary).yellow();
@@ -244,7 +254,7 @@ pub(crate) fn format_summary(commit: &ConventionalCommit) -> String {
 
 #[cfg(test)]
 mod test {
-    use crate::conventional::commit::{format_summary, verify, Commit};
+    use crate::conventional::commit::{format_summary, verify, Commit, CommitConfig};
 
     use chrono::NaiveDateTime;
     use cmd_lib::run_fun;
@@ -440,6 +450,36 @@ mod test {
         assert_that!(summary).is_equal_to("fix: this is the message".to_string());
     }
 
+    #[test]
+    fn should_toggle_changelog_omission() {
+        // Arrange
+        let mut config = CommitConfig::new("Omittable Changes");
+
+        // Assert
+        assert!(
+            !&config.omit_from_changelog,
+            "expected CommitConfig::omit_from_changelog to be falsy unless explicitly set"
+        );
+
+        // Act
+        config.omit_from_changelog = true;
+
+        // Assert
+        assert!(
+            &config.omit_from_changelog,
+            "CommitConfig::omit_from_changelog should be truthy after calling CommitConfig::omit"
+        );
+
+        // Act
+        config.omit_from_changelog = false;
+
+        // Assert
+        assert!(
+            !&config.omit_from_changelog,
+            "CommitConfig::omit_from_changelog should be falsy after calling CommitConfig::include"
+        );
+    }
+
     #[sealed_test]
     fn should_map_conventional_commit() {
         // Arrange


Test patch:
diff --git a/tests/cog_tests/changelog.rs b/tests/cog_tests/changelog.rs
index 54c74f4..3118470 100644
--- a/tests/cog_tests/changelog.rs
+++ b/tests/cog_tests/changelog.rs
@@ -362,7 +362,7 @@ fn get_changelog_from_tag_to_tagged_head() -> Result<()> {
 }
 
 #[sealed_test]
-fn get_changelog_whith_custom_template() -> Result<()> {
+fn get_changelog_with_custom_template() -> Result<()> {
     // Arrange
     let crate_dir = env!("CARGO_MANIFEST_DIR");
     let template = PathBuf::from(crate_dir).join("tests/cog_tests/template.md");
@@ -437,3 +437,66 @@ fn get_changelog_whith_custom_template() -> Result<()> {
     );
     Ok(())
 }
+
+#[sealed_test]
+/// Test that the `omit_from_changelog` configuration
+/// directive is honored if/when it is specified for
+/// a given commit type.
+fn ensure_omit_from_changelog_is_honored() -> Result<()> {
+    // Arrange
+    git_init()?;
+
+    let cog_toml = indoc!(
+        "[changelog]
+        remote = \"github.com\"
+        repository = \"test\"
+        owner = \"test\"
+
+        [commit_types]
+        wip = { changelog_title = \"Work In Progress\", omit_from_changelog = false }"
+    );
+
+    let _setup = (
+        run_cmd!(echo $cog_toml > cog.toml;)?,
+        fs::read_to_string("cog.toml")?,
+        git_commit("chore: init")?,
+        git_commit("wip(some-scope): getting there")?,
+        git_tag("1.0.0")?,
+    );
+
+    let changelog = Command::cargo_bin("cog")?
+        .arg("changelog")
+        // Assert
+        .assert()
+        .success();
+
+    let changelog = changelog.get_output();
+    let changelog = String::from_utf8_lossy(&changelog.stdout);
+
+    assert!(
+        changelog.as_ref().contains("#### Work In Progress"),
+        "Expected changelog to contain a \"Work In Progress\" entry but got:\n\n{}",
+        changelog.as_ref()
+    );
+
+    let cog_toml = cog_toml.replace("omit_from_changelog = false", "omit_from_changelog = true");
+
+    run_cmd!(echo $cog_toml > cog.toml;)?;
+
+    let changelog = Command::cargo_bin("cog")?
+        .arg("changelog")
+        // Assert
+        .assert()
+        .success();
+
+    let changelog = changelog.get_output();
+    let changelog = String::from_utf8_lossy(&changelog.stdout);
+
+    assert!(
+        !changelog.as_ref().contains("#### Work In Progress"),
+        "Expected \"Work In Progress\" entry to be omitted from changelog but got:\n\n{}",
+        changelog.as_ref()
+    );
+
+    Ok(())
+}