feat: filter footers

[jan-ferdinand]

Description

Enable footer-based commit parsing. The idea is to add functionality like

commit_parsers = [
  { footer = "Hello: world", skip = true },
]

I would appreciate some guidance – is this useful, desired, worth it, …?

Motivation and Context

In order to keep my changelogs concise, I would like to ignore certain commits. I would like to specify whether a commit should be ignored in the commit message. A somewhat elegant way to achieve this (I think) would be to include a footer like changelog: ignore.
(If you have a better way to approach this, I'm happy to learn of it.)

How Has This Been Tested?

Append existing tests that create sample changelogs.

The test changelog_generator_split_commits is failing. It appears that a footer's value (but not the key) makes its way into the changelog. This might be because the footer's key is interpreted as a conventional commit type. I would appreciate some guidance here.

Possible Limitations

The conventional commits' specification and the reference implementation of its parser are currently out of sync. This might mean that the commit parser {footer = "Hello: world"} does not work (even though it should), whereas the commit parser {footer = "Hello:world"} does work (even though omitting the space goes against the specification).

Some relevant issues concerning footer parsing are:

• Grammar does not enforce whitespace after : token in footers conventional-commits/parser#47
• Grammar cannot read git trailers conventional-commits/parser#41
• fix: footer seperator : requires space crate-ci/git-conventional#54

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 formatted the code with rustfmt.
• I checked the lints with clippy.
• I have added tests to cover my changes.
• All new and existing tests passed.

[welcome]

Thanks for opening this pull request! Please check out our contributing guidelines! ⛰️

[orhun]

Thanks for the PR!

I would appreciate some guidance – is this useful, desired, worth it, …?
A somewhat elegant way to achieve this (I think) would be to include a footer like changelog: ignore.

I'm wondering if the same can be achieved with body which git-cliff already supports filtering.

I'm also wondering if this is useful for #382. But I'm guessing "This commit reverts" is not a part of the footer.

The test changelog_generator_split_commits is failing. It appears that a footer's value (but not the key) makes its way into the changelog. This might be because the footer's key is interpreted as a conventional commit type. I would appreciate some guidance here.

That is the expected behavior so I recommend you to either add a new CommitParser to changelog_generator_split_commits to skip that commit or update the expected result accordingly to include that commit in the changelog.

Overall, I think this might be useful. Can you update the documentation about the existence of footer filter and also add a test fixture to verify it's behavior?

[orhun]

ping @jan-ferdinand 🙂

[jan-ferdinand]

Other things have come up and are more pressing right now; I'll come back to this. Apologies if that creates inconveniencies.

[orhun]

No worries!

[orhun]

ping

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 36.51%. Comparing base (dabe716) to head (df0279f).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #569      +/-   ##
==========================================
+ Coverage   36.29%   36.51%   +0.22%     
==========================================
  Files          19       19              
  Lines        1455     1460       +5     
==========================================
+ Hits          528      533       +5     
  Misses        927      927              

Flag	Coverage Δ
unit-tests	36.51% <100.00%> (+0.22%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jan-ferdinand]

Thanks for ping & patience. I have made the following changes, most of which were requested or guided towards:

• rebase onto current main
• fix test changelog_generator_split_commits
• add fixture
• add some documentation

I would like to have some additional guidance on the following points:

• Is the amount of documentation enough? It seems to be very little, but I couldn't readily identify more places where it makes sense to add additional documentation.
• The conventional commit reference parser being out of sync with the specification is annoying for this PR. It requires users that want to use the suggested feature to write parsers of the form { footer = "^key:value } (which goes against the spec), or, to be a bit more future proof { footer = ^key: ?value } (which is confusing). Does it still make sense to go ahead with the suggested feature despite the possible confusion?
• I have left comments briefly discussing the out-of-syncedness of spec & parser in two places, one in the fixture and one in the spec. Does this make sense, or would you rather I remove or merge these? The random : ? in the footer parser seems like the kind of regex-odditiy that future devs (including us) will trip over, but I'm not sure it's worth the current verbosity.

[orhun]

Thanks for picking up on this!

Is the amount of documentation enough? It seems to be very little, but I couldn't readily identify more places where it makes sense to add additional documentation.

I think it is good enough! I will mention this feature in the news so it should be visible enough.

Does it still make sense to go ahead with the suggested feature despite the possible confusion?

Yes, we can go ahead with it. Alternatively, I'm not sure if there are any actions needed but, you can report this to git-conventional (which git-cliff uses for parsing the commits) FWIW.

I have left comments briefly discussing the out-of-syncedness of spec & parser in two places, one in the fixture and one in the spec. Does this make sense, or would you rather I remove or merge these?

I would rather have them as comment like they are now. Good to have the references 👍🏼

[orhun]

LGTM, there is only one conflict that needs to be resolved then we are good to go!

[jan-ferdinand]

Thanks for picking up on this!

Took me long enough. 🤷 Thanks for your patience!

I think it is good enough! I will mention this feature in the news so it should be visible enough.

👌

Yes, we can go ahead with it. Alternatively, I'm not sure if there are any actions needed but, you can report this to git-conventional (which git-cliff uses for parsing the commits) FWIW.

I had opened a PR in trying to resolve this issue. The maintainer chose to stay consistent with the reference implemenation rather than deviating from it, a decision I can understand.

I would rather have them as comment like they are now. Good to have the references 👍🏼

👍

Thanks for your guidance. 😊 I'm going to rebase & force-push in hopes of removing the blocking conflict.

Edit: I have also squashed all commits and changed the commit message. Let me know if any of that goes against this project's guidelines. 😌