-
-
Notifications
You must be signed in to change notification settings - Fork 198
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(config)!: revamp configuration #559
base: main
Are you sure you want to change the base?
Conversation
Thanks for opening this pull request! Please check out our contributing guidelines! ⛰️ |
44f1335
to
f677ceb
Compare
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #559 +/- ##
==========================================
- Coverage 41.52% 39.04% -2.47%
==========================================
Files 15 18 +3
Lines 1072 1145 +73
==========================================
+ Hits 445 447 +2
- Misses 627 698 +71
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
14d2f11
to
25121ec
Compare
d0e907b
to
bfb7fdf
Compare
339b9bd
to
5ce1b9e
Compare
5ce1b9e
to
1548930
Compare
Added a subcommand migrate-config to migrate from the old to the new configuration format.
fe6d2bd
to
a483d71
Compare
a483d71
to
f69dfe4
Compare
Hi @orhun, |
Inviting @MarcoIeni @alerque @kenji-miyake @tranzystorekk for thoughts and review 🤗 |
# https://keats.github.io/tera/docs/#introduction | ||
body = """ | ||
# A Tera template to be rendered for each release in the changelog (see https://keats.github.io/tera/docs/#introduction). | ||
body_template = """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't we make header
a template too and remove _template
from all the changelog parts?
Probably this suggestion would have been better when discussing the issue, but I didn't pay enough attention, so sorry about that!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that would certainly be a good idea. I didn't do that yet, because I wanted to keep the changes distinct. But I can add it to this PR.
# remove the leading and trailing whitespace from the templates | ||
trim = true | ||
# Whether to remove leading and trailing whitespaces from all lines of the changelog's body. | ||
trim_body_whitespace = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't this also trim new lines?
Maybe trim_body
is enough as a name. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless I misunderstand the code in git-cliff-core/src/template.rs:34, this option does not remove newlines.
[commit] | ||
# Whether to parse commits according to the conventional commits specification. | ||
# Sets the commits' `group` (= `type`), `scope`, `message` (= `description`), `body`, `breaking`, `breaking_description` and `footers`. | ||
parse_conventional_commits = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think parse_
adds much more clarity here, so imo we could also avoid adding it to spare this breaking change.
But this comes to personal taste :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for the name, but this option feels weirdly interlinked with exclude_unconventional_commits
, a single option like conventional_commits=<strict|enabled|disabled>
where strict
means "also exclude unconventional" seems more intuitive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the enum option, but we'd need a 4th option for requiring all commits to be unconventional in the future. I think strict
would be more suitable for that 4th option. But that means we need another name for "exclude unconventional".
# Sets the commits' `group` (= `type`), `scope`, `message` (= `description`), `body`, `breaking`, `breaking_description` and `footers`. | ||
parse_conventional_commits = false | ||
# Whether to exclude commits that do not match the conventional commits specification from the changelog. | ||
exclude_unconventional_commits = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like exclude
more than filter
. It's more clear 👍
Since we are already in the [commit]
section, should we drop _commits
? Same for parse_conventional_commits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dislike removing the suffix _commits
because the section you are currently in isn't always obvious with toml. Given that config files can be quite large, the section header can be off screen.
|
||
[commit] | ||
# Regex to select git tags that should be excluded from the changelog. | ||
exclude_tags_pattern = "v0.1.0-beta.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we pick one between exclude_
and skip_
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exclude_
and skip_
have different meanings. Changes from excluded tags are completely removed from the changelog, whereas changes from skipped tags are contained in the next release thereafter. It certainly is not ideal naming but I could not find a better option.
Description
Implementation of #541. This PR aims to clean up the configuration of git-cliff in several ways.
Note: Every option was refactored in a separate commit to allow for detailed reviews and targeted changes.
Motivation and Context
Currently the configuration is fairly convoluted and difficult to understand. For example, several options like
git.skip_tags
andgit.ignore_tags
have names that are not descriptive and can easily be confused.How Has This Been Tested?
All existing tests are successful for every commit in this PR. This PR does not contain any functional changes.
Screenshots / Logs (if applicable)
Types of Changes
Checklist: