-
-
Notifications
You must be signed in to change notification settings - Fork 301
fix(commands/bump): prevent using incremental changelog when it is set to false in config #996
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
fix(commands/bump): prevent using incremental changelog when it is set to false in config #996
Conversation
661a140
to
d4894e5
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@ ## master #996 +/- ## ========================================== + Coverage 97.33% 97.45% +0.11% ========================================== Files 42 55 +13 Lines 2104 2398 +294 ========================================== + Hits 2048 2337 +289 - Misses 56 61 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
...t to false in config
d4894e5
to
45b9352
Compare
Just opening the debate: wouldn't it be easier and more consistent to fix/align the default bump
behavior to the default settings (even if this is a breaking change) ?
By adding a fourth settings source of trust (defaults, user settings, cli flags and now mutated settings, fifth if you add the fact that plugins can override some settings), I feel that we are fixing a symptom but making the problem more complex.
Just opening the debate: wouldn't it be easier and more consistent to fix/align the default
bump
behavior to the default settings (even if this is a breaking change) ?By adding a fourth settings source of trust (defaults, user settings, cli flags and now mutated settings, fifth if you add the fact that plugins can override some settings), I feel that we are fixing a symptom but making the problem more complex.
Yeah, I think that would be a better way to address the issue if possible. The implementation would be easier and also could prevent overriding the behaviors from multiple sources when running the bump --changelog
.
Just opening the debate: wouldn't it be easier and more consistent to fix/align the default bump behavior to the default settings (even if this is a breaking change) ?
Could you make a proposal for this? It sounds interesting, would be nice if we can simplify the settings
I'm a bit confused here. I thought we could solve it by reading the value from config?
I thought we could solve it by reading the value from config?
I think there are total three combinations of configurations here:
- user didn't specify
changelog_incremental
, the Config would store the value asFalse
and the value passing intoChangelog
should beTrue
- user did specify
changelog_increamental: true
, the Config would store the value asTrue
, so the value passing intoChangelog
would also beTrue
- user did specify
changelog_increamental: false
, the Config would store the value asFalse
, so the value passing intoChangelog
would also beFalse
The 1st & 3rd cases would cause ambiguity when we're reading the value from Config
, we couldn't tell if the False value is set from the default value or user configuration, according to different source of the value we would take different value to pass into the initialization of Changelog
. To address this, I introduce one more property that trace the value set from users to help us differentiate them in this PR.
we couldn't tell if the False value is set from the default value or user configuration,
I thought the one from the config should overwrite the default. Or did I miss anything?
Hello,
Do you guys have any news on this fix ?
This setting is blocking the changelog_merge_prerelease
option, as the arg incremental
take the precedence, causing us to split the bump and the changelog generation in order to have proper release changelog with every changes from release-x to release-x+1 concatenated, and not with every intermediate pre-release.
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 probably won't say I love this idea, but it is an acceptable workaround before we introduce a better config system. @josix, could you please rebase it and add some tests to verify it?
Uh oh!
There was an error while loading. Please reload this page.
Fix #883
Description
Since the default behavior of
cz bump --changelog
is to write incremental changelog, which is different from the default value ofchangelog_incremental
, it is hard to know the value is set from user or the default setting, To address it, I created one more member inBaseConfig
to record which property is defined from users so that we could distinguish the value is from default setting or users.Checklist
./scripts/format
and./scripts/test
locally to ensure this change passes linter check and testExpected behavior
cz bump --changelog
should work asfalse
when user configurechangelog_incremental
tofalse
.Steps to Test This Pull Request
Modify the CHANGELOG file
image
Configure
pyproject.toml
withchangelog_incremental = false
Run
cz bump --changelog
Check the CHANGELOG, which the new added content should be replaced
image
Additional context
ditto.