Skip to content
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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Cyclonit
Copy link
Contributor

@Cyclonit Cyclonit commented Mar 16, 2024

Description

Implementation of #541. This PR aims to clean up the configuration of git-cliff in several ways.

  • Improve names for configuration options to improve clarity.
  • Improve descriptions and documentation for options.
  • Restructure configuration options to improve clarity.

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 and git.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

  • 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.

@Cyclonit Cyclonit requested a review from orhun as a code owner March 16, 2024 16:57
Copy link

welcome bot commented Mar 16, 2024

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

@Cyclonit Cyclonit force-pushed the feature/revamp_configuration branch 2 times, most recently from 44f1335 to f677ceb Compare March 16, 2024 17:00
@codecov-commenter
Copy link

codecov-commenter commented Mar 16, 2024

Codecov Report

Attention: Patch coverage is 23.44498% with 160 lines in your changes are missing coverage. Please review.

Project coverage is 39.04%. Comparing base (d53b905) to head (f69dfe4).

Files Patch % Lines
git-cliff/src/config.rs 0.00% 44 Missing ⚠️
git-cliff/src/lib.rs 0.00% 37 Missing ⚠️
git-cliff-core/src/config/embed.rs 0.00% 26 Missing ⚠️
git-cliff-core/src/changelog.rs 51.73% 14 Missing ⚠️
git-cliff-core/src/config/migrate.rs 0.00% 12 Missing ⚠️
git-cliff-core/src/config/models_v2.rs 56.53% 10 Missing ⚠️
git-cliff-core/src/config/parsing.rs 43.75% 9 Missing ⚠️
git-cliff/src/main.rs 0.00% 7 Missing ⚠️
git-cliff-core/src/commit.rs 92.86% 1 Missing ⚠️

❗ 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     
Flag Coverage Δ
unit-tests 39.04% <23.45%> (-2.47%) ⬇️

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.

@Cyclonit Cyclonit force-pushed the feature/revamp_configuration branch 5 times, most recently from 14d2f11 to 25121ec Compare March 19, 2024 06:23
@Cyclonit Cyclonit marked this pull request as draft March 19, 2024 06:26
@Cyclonit Cyclonit force-pushed the feature/revamp_configuration branch 7 times, most recently from d0e907b to bfb7fdf Compare March 24, 2024 11:32
@Cyclonit Cyclonit force-pushed the feature/revamp_configuration branch 7 times, most recently from 339b9bd to 5ce1b9e Compare April 14, 2024 06:11
@Cyclonit Cyclonit force-pushed the feature/revamp_configuration branch from 5ce1b9e to 1548930 Compare April 14, 2024 08:28
@Cyclonit Cyclonit force-pushed the feature/revamp_configuration branch 12 times, most recently from fe6d2bd to a483d71 Compare April 15, 2024 07:18
@Cyclonit Cyclonit force-pushed the feature/revamp_configuration branch from a483d71 to f69dfe4 Compare April 15, 2024 08:00
@Cyclonit Cyclonit marked this pull request as ready for review April 15, 2024 10:57
@Cyclonit
Copy link
Contributor Author

Hi @orhun,
I consider this PR to be ready for review. All fixtures are green. The only thing remaining is fixing the linting messages that are caused by the deprecated v1 config classes.

@orhun
Copy link
Owner

orhun commented May 11, 2024

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 = """
Copy link
Contributor

@MarcoIeni MarcoIeni May 11, 2024

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!

Copy link
Contributor Author

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
Copy link
Contributor

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. 🤔

Copy link
Contributor Author

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
Copy link
Contributor

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 :)

Copy link
Contributor

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

Copy link
Contributor Author

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
Copy link
Contributor

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

Copy link
Contributor Author

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"
Copy link
Contributor

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_?

Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants