feat(commit)!: pass footer token and separator to template #97

hawkw · 2022-06-24T16:48:03Z

Description

This branch changes the Serialize impl for Commit to output conventional
commit footers as structured JSON objects, rather than as just the footer's
value string.

See hawkw/git-cliff@93779b2 for a discussion of
the implementation details of this change.

Motivation and Context

As discussed in issue #96, using footers in changelog templates is currently not
practical in most cases, since there is currently no way to determine which
footer a value belongs to, and no way to access the name of the footer.

This PR implements the approach suggested by @orhun in #96 (comment).
Additionally, I added a section to the README documentation discussing the
structure of the footers passed to templates.

Fixes #96

How Has This Been Tested?

I added a new test in commit.rs that conventional commits with footers have a
footers iterator containing the correct footers. In addition, I did some
manual testing to test the use of footers in templates --- see the next section
for details.

Screenshots / Output (if appropriate):

To manually verify that everthing works end-to-end, I temporarily modified
examples/detailed.toml to output footers in the template:

diff --git a/examples/detailed.toml b/examples/detailed.toml
index b6b57fd..6fc2252 100644
--- a/examples/detailed.toml
+++ b/examples/detailed.toml
@@ -24,6 +24,9 @@ body = """
     ### {{ group | upper_first }}
     {% for commit in commits %}
         - {{ commit.message | upper_first }} ([{{ commit.id | truncate(length=7, end="") }}]({{ commit.id }}))\
+          {% for footer in commit.footers -%}
+            , {{ footer.token }}{{ footer.separator }} {{ footer.value }}\
+          {% endfor %}
     {% endfor %}
 {% endfor %}\n
 """

Generating a test changelog with that template now outputs footers, as expected:

# Changelog

All notable changes to this project will be documented in this file.

## [unreleased]

### Documentation

- Clarify that `--tag` argument can be an unexisting tag ([d540f5d](d540f5d8938bc84b01b4fafaa69c3290eb72cd08))

- Discuss footers in README ([a54bca5](a54bca57acc91a886d52bb9d3d87866eb5c18f32)), Signed-off-by: Eliza Weisman <eliza@buoyant.io>


### Features

- Support external commands for commit preprocessors (#86) ([7d0786c](7d0786ca55423950f0779de4e6a907fc25ae203a))

- Support changing commit scope with `commit_parsers` (#94) ([e220768](e22076843b91be3680617db5686e070dedcfef29)), Co-authored-by: Orhun Parmaksız <orhunparmaksiz@gmail.com>

...

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation (no code change)
Refactor (refactoring production code)
Other

Checklist:

My code follows the code style of this project.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.

Currently, when a conventional commit has footers, only the footers' values (the part after the separator token, such as `:`) are passed to the template. This means that when multiple footers, such as `Signed-off-by:` and `Co-authored-by:` are present, it isn't currently possible for the template to determine the name of the footer. This makes actually using data from footers in templates impractical in most cases. This commit fixes this by changing the `Serialize` impl for `Commit` to pass the commit's footers as a structured object rather than a string. The structured `Footer` type includes the footer's token (which is what `git_conventional` calls the name preceding the separator token), the separator, and the value. I didn't make the new `Footer` type and `Commit::footers` method public, because it isn't strictly necessary to add them to the `git-cliff-core` public API to fix this issue. However, we can make them public in a follow-up PR if this is considered useful. Fixes orhun#96 BREAKING CHANGE: This changes type of the `commit.footers` array exposed to templates. Currently, when a template uses `commit.footers`, it can treat the values as strings. After this change, the footer object will need to have its fields unpacked in order to use them. However, the impact of this breakage is probably not that severe, since it's not really practical to use footers in templates with the current system.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

codecov-commenter · 2022-06-25T22:07:28Z

Codecov Report

Merging #97 (9f78b0c) into main (175f7d7) will increase coverage by 3.26%.
The diff coverage is 85.72%.

@@            Coverage Diff             @@
##             main      #97      +/-   ##
==========================================
+ Coverage   40.15%   43.40%   +3.26%     
==========================================
  Files          15       15              
  Lines         969     1007      +38     
  Branches      252      252              
==========================================
+ Hits          389      437      +48     
+ Misses        456      444      -12     
- Partials      124      126       +2

Impacted Files	Coverage Δ
git-cliff-core/src/commit.rs	`73.65% <85.72%> (+3.99%)`	⬆️
git-cliff-core/src/release.rs	`20.00% <0.00%> (+10.00%)`	⬆️
git-cliff-core/src/config.rs	`35.00% <0.00%> (+18.34%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 175f7d7...9f78b0c. Read the comment docs.

orhun

Had a couple of nitpicks here and there but looks great! 💯

Can you also add test fixtures for this change as well? I might do it after merge if you don't want to include in the PR, no problem.

git-cliff-core/src/commit.rs

README.md

orhun · 2022-06-25T23:04:10Z

diff --git a/examples/detailed.toml b/examples/detailed.toml
index b6b57fd..6fc2252 100644
--- a/examples/detailed.toml
+++ b/examples/detailed.toml
@@ -24,6 +24,9 @@ body = """
     ### {{ group | upper_first }}
     {% for commit in commits %}
         - {{ commit.message | upper_first }} ([{{ commit.id | truncate(length=7, end="") }}]({{ commit.id }}))\
+          {% for footer in commit.footers -%}
+            , {{ footer.token }}{{ footer.separator }} {{ footer.value }}\
+          {% endfor %}
     {% endfor %}
 {% endfor %}\n
 """

Ah, also include this change in the PR please. It seems like a good way to demonstrate how footers can be used within template.

orhun · 2022-06-30T07:21:02Z

Hey @hawkw, can you take a look at this PR soon? I would like to include it in the next release of git-cliff if possible 🐻

hawkw · 2022-07-01T18:04:15Z

Sorry, I didn't have a chance to follow up sooner. I'll go ahead and address the remaining feedback, thanks!

hawkw · 2022-07-01T18:06:15Z

diff --git a/examples/detailed.toml b/examples/detailed.toml
index b6b57fd..6fc2252 100644
--- a/examples/detailed.toml
+++ b/examples/detailed.toml
@@ -24,6 +24,9 @@ body = """
     ### {{ group | upper_first }}
     {% for commit in commits %}
         - {{ commit.message | upper_first }} ([{{ commit.id | truncate(length=7, end="") }}]({{ commit.id }}))\
+          {% for footer in commit.footers -%}
+            , {{ footer.token }}{{ footer.separator }} {{ footer.value }}\
+          {% endfor %}
     {% endfor %}
 {% endfor %}\n
 """

Ah, also include this change in the PR please. It seems like a good way to demonstrate how footers can be used within template.

Sure, happy to do that! One quick question, though: is there an example repository you're using to generate the example changelogs in the README? I would want to re-generate the "detailed" example now that the template has changed.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

hawkw · 2022-07-01T18:14:59Z

Can you also add test fixtures for this change as well? I might do it after merge if you don't want to include in the PR, no problem.

I added some tests for commits with footers in commit.rs, here:

git-cliff/git-cliff-core/src/commit.rs

Lines 354 to 404 in 9f78b0c

    
           	#[test] 
        
           	fn conventional_footers() { 
        
           		let cfg = crate::config::GitConfig { 
        
           			conventional_commits: Some(true), 
        
           			..Default::default() 
        
           		}; 
        
           		let test_cases = vec![ 
        
           			( 
        
           				Commit::new( 
        
           					String::from("123123"), 
        
           					String::from( 
        
           						"test(commit): add test\n\nSigned-off-by: Test User \ 
        
           						 <test@example.com>", 
        
           					), 
        
           				), 
        
           				vec![Footer { 
        
           					token:     "Signed-off-by", 
        
           					separator: ":", 
        
           					value:     "Test User <test@example.com>", 
        
           					breaking:  false, 
        
           				}], 
        
           			), 
        
           			( 
        
           				Commit::new( 
        
           					String::from("123124"), 
        
           					String::from( 
        
           						"fix(commit): break stuff\n\nBREAKING CHANGE: This commit \ 
        
           						 breaks stuff\nSigned-off-by: Test User <test@example.com>", 
        
           					), 
        
           				), 
        
           				vec![ 
        
           					Footer { 
        
           						token:     "BREAKING CHANGE", 
        
           						separator: ":", 
        
           						value:     "This commit breaks stuff", 
        
           						breaking:  true, 
        
           					}, 
        
           					Footer { 
        
           						token:     "Signed-off-by", 
        
           						separator: ":", 
        
           						value:     "Test User <test@example.com>", 
        
           						breaking:  false, 
        
           					}, 
        
           				], 
        
           			), 
        
           		]; 
        
           		for (commit, footers) in &test_cases { 
        
           			let commit = commit.process(&cfg).expect("commit should process"); 
        
           			assert_eq!(&commit.footers().collect::<Vec<_>>(), footers); 
        
           		} 
        
           	}

Are there other tests you're referring to? I'm happy to add any other tests that are necessary for this change, but I wasn't sure where else you would want to test this.

orhun · 2022-07-06T10:47:49Z

Sure, happy to do that! One quick question, though: is there an example repository you're using to generate the example changelogs in the README? I would want to re-generate the "detailed" example now that the template has changed.

Ah, good point. I made it public: https://github.com/orhun/git-cliff-readme-example

orhun · 2022-07-06T10:51:41Z

I added some tests for commits with footers in commit.rs, here:

Yup, it looks good 👍🏼

Are there other tests you're referring to? I'm happy to add any other tests that are necessary for this change, but I wasn't sure where else you would want to test this.

There are fixture tests in .github/fixtures for testing the functionality of git-cliff by running it with a pre-defined commit history. It is not super important, but nice to have. Like I said, I can work on it after merging the PR if that's something that you'd not prefer to add 🙂

hawkw · 2022-07-06T17:44:57Z

Are there other tests you're referring to? I'm happy to add any other tests that are necessary for this change, but I wasn't sure where else you would want to test this.

There are fixture tests in .github/fixtures for testing the functionality of git-cliff by running it with a pre-defined commit history. It is not super important, but nice to have. Like I said, I can work on it after merging the PR if that's something that you'd not prefer to add slightly_smiling_face

Oh, I see, I missed those. I'll figure out how to add those tests as well.

orhun · 2022-07-12T12:33:33Z

Oh, I see, I missed those. I'll figure out how to add those tests as well.

I just added fixture tests and made some tweaks to the detailed.toml example. Merging, thanks for the PR!

hawkw added 2 commits June 24, 2022 09:16

docs(README): discuss footers in README

a54bca5

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

hawkw requested a review from orhun as a code owner June 24, 2022 16:48

orhun requested changes Jun 25, 2022

View reviewed changes

git-cliff-core/src/commit.rs Outdated Show resolved Hide resolved

git-cliff-core/src/commit.rs Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

hawkw added 3 commits July 1, 2022 11:09

docs(examples): Add footers to detailed.toml

ee9e563

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

refac(commit): address review feedback

c5dbc11

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

docs(README): address README review feedback

9f78b0c

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

hawkw requested a review from orhun July 1, 2022 18:15

orhun added 2 commits July 12, 2022 14:07

refactor(example): update detailed example about newline issues

9f33f39

test(fixture): add test fixture for commit footers

26a81e0

orhun approved these changes Jul 12, 2022

View reviewed changes

orhun changed the title ~~fix(commit): pass footer token and separator to template~~ feat(commit)!: pass footer token and separator to template Jul 12, 2022

orhun merged commit 0bf499e into orhun:main Jul 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(commit)!: pass footer token and separator to template #97

feat(commit)!: pass footer token and separator to template #97

hawkw commented Jun 24, 2022 •

edited

Loading

codecov-commenter commented Jun 25, 2022 •

edited

Loading

orhun left a comment

orhun commented Jun 25, 2022

orhun commented Jun 30, 2022

hawkw commented Jul 1, 2022

hawkw commented Jul 1, 2022

hawkw commented Jul 1, 2022 •

edited

Loading

orhun commented Jul 6, 2022

orhun commented Jul 6, 2022

hawkw commented Jul 6, 2022

orhun commented Jul 12, 2022

feat(commit)!: pass footer token and separator to template #97

feat(commit)!: pass footer token and separator to template #97

Conversation

hawkw commented Jun 24, 2022 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Screenshots / Output (if appropriate):

Types of changes

Checklist:

codecov-commenter commented Jun 25, 2022 • edited Loading

Codecov Report

orhun left a comment

Choose a reason for hiding this comment

orhun commented Jun 25, 2022

orhun commented Jun 30, 2022

hawkw commented Jul 1, 2022

hawkw commented Jul 1, 2022

hawkw commented Jul 1, 2022 • edited Loading

orhun commented Jul 6, 2022

orhun commented Jul 6, 2022

hawkw commented Jul 6, 2022

orhun commented Jul 12, 2022

hawkw commented Jun 24, 2022 •

edited

Loading

codecov-commenter commented Jun 25, 2022 •

edited

Loading

hawkw commented Jul 1, 2022 •

edited

Loading