-
-
Notifications
You must be signed in to change notification settings - Fork 208
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
doc(config): update comments for all configuration options #1057
base: main
Are you sure you want to change the base?
Conversation
96ef88f
to
cdddd9d
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1057 +/- ##
==========================================
- Coverage 41.90% 41.85% -0.05%
==========================================
Files 21 21
Lines 1790 1790
==========================================
- Hits 750 749 -1
- Misses 1040 1041 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
cdddd9d
to
a0ae4cd
Compare
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.
Thanks, great cleanup! 🎉
I had some comments, it would be nice to update the other occurrences based on those as well :)
ignore_tags = "" | ||
# sort the tags topologically | ||
# Order releases topologically instead of chronologically. |
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.
# Order releases topologically instead of chronologically. | |
# Order tags topologically instead of chronologically. |
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.
Why tags
instead of releases
? Doesn't it affect the release order? The description should reflect the options effect and not how it is implemented.
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.
We are using this flag to sort the tags, which then affects the order of the releases.
git-cliff/git-cliff-core/src/repo.rs
Lines 421 to 423 in 5f3a3d0
if !topo_order { | |
tags.sort_by(|a, b| a.0.time().seconds().cmp(&b.0.time().seconds())); | |
} |
So I think using "tags" would be more correct here.
The description should reflect the options effect and not how it is implemented.
I think the user should be aware of tags vs releases when using a git automation tool like this.
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.
But a user wants to know how the config affects the final result. And there it affects the order of releases, by sorting the tags. The variable tags
in your code reference could more accurately be referred to as release_tags
. Because those tags represent releases. I am willing to be overruled, but these are my last two cents ;)
# How to order commits in each group/release within the changelog. | ||
# allowed values: newest, oldest |
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.
# How to order commits in each group/release within the changelog. | |
# allowed values: newest, oldest | |
# Whether to order commits in each group/release by newest/oldest. |
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 checked with others and they agreed that putting the options on a separate line makes them being the valid options easier to understand. What reason is there to keep in short?
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.
It's not a big deal imo - I just didn't want to spend a whole line for explaining possible values for this option.
If we go with a new line, this might imply that we are following this format for the other options in the future.
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.
Would that not be a good idea?
a0ae4cd
to
d10937c
Compare
Description
Updated the comments in all instances of
config.toml
andconfig.rs
.Motivation and Context
Some of the comments were unclear, had inconsistent punctation or wording.
How Has This Been Tested?
no functional changes
Screenshots / Logs (if applicable)
does not apply
Types of Changes
Checklist: