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

fix(commands/bump): prevent using incremental changelog when it is set to false in config #996

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

josix
Copy link
Contributor

@josix josix commented Feb 29, 2024

Fix #883

Description

Since the default behavior of cz bump --changelog is to write incremental changelog, which is different from the default value of changelog_incremental, it is hard to know the value is set from user or the default setting, To address it, I created one more member in BaseConfig to record which property is defined from users so that we could distinguish the value is from default setting or users.

Checklist

  • Add test cases to all the changes you introduce
  • Run ./scripts/format and ./scripts/test locally to ensure this change passes linter check and test
  • Test the changes on the local machine manually
  • Update the documentation for the changes

Expected behavior

cz bump --changelog should work as false when user configure changelog_incremental to false.

Steps to Test This Pull Request

  1. Modify the CHANGELOG file
    image

  2. Configure pyproject.toml with changelog_incremental = false

  3. Run cz bump --changelog

  4. Check the CHANGELOG, which the new added content should be replaced
    image

Additional context

ditto.

@josix josix force-pushed the fix/changelog-default-changelog-incremental branch from 661a140 to d4894e5 Compare February 29, 2024 18:01
Copy link

codecov bot commented Feb 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.45%. Comparing base (120d514) to head (d439c38).
Report is 231 commits behind head on master.

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     
Flag Coverage Δ
unittests 97.45% <100.00%> (+0.11%) ⬆️

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.

@josix josix force-pushed the fix/changelog-default-changelog-incremental branch from d4894e5 to 45b9352 Compare February 29, 2024 18:25
@josix josix marked this pull request as ready for review March 8, 2024 08:34

@property
def settings(self) -> Settings:
return self._settings

@property
def mutated_settings(self) -> Settings:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a docstring to this function explaining it a bit further? thanks
Right now it seems it does the same as mutated_settings, but I think the intention is different, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I've added it. Could you help check it, thanks!

@josix josix requested a review from woile March 8, 2024 11:40
@noirbizarre
Copy link
Member

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.

@josix
Copy link
Contributor Author

josix commented Mar 11, 2024

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.

@woile
Copy link
Member

woile commented Mar 15, 2024

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

@Lee-W
Copy link
Member

Lee-W commented Mar 30, 2024

I'm a bit confused here. I thought we could solve it by reading the value from config?

@josix
Copy link
Contributor Author

josix commented Apr 1, 2024

I thought we could solve it by reading the value from config?

I think there are total three combinations of configurations here:

  1. user didn't specify changelog_incremental, the Config would store the value as False and the value passing into Changelog should be True
  2. user did specify changelog_increamental: true, the Config would store the value as True, so the value passing into Changelog would also be True
  3. user did specify changelog_increamental: false, the Config would store the value as False, so the value passing into Changelog would also be False

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.

@Lee-W
Copy link
Member

Lee-W commented Apr 2, 2024

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?

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

Successfully merging this pull request may close these issues.

changelog_incremental is set as default when calling changelog from cz bump
4 participants