-
-
Notifications
You must be signed in to change notification settings - Fork 304
fix(1694): changelog_merge_prerelease not working on cz bump #1700
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
base: v4-11-0
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4-11-0 #1700 +/- ##
==========================================
Coverage ? 97.87%
==========================================
Files ? 60
Lines ? 2639
Branches ? 0
==========================================
Hits ? 2583
Misses ? 56
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
woile
left a comment
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.
Very nice, thanks!
tests/commands/test_bump_command.py
Outdated
| cli.main() | ||
|
|
||
| testargs = ["cz", "bump", "--changelog"] | ||
|
|
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.
nit: remove this blank line
| if during_version_bump and rules.merge_prereleases: | ||
| current_tag = None | ||
| else: | ||
| current_tag = get_commit_tag(commits[0], tags) if commits else None |
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.
| if during_version_bump and rules.merge_prereleases: | |
| current_tag = None | |
| else: | |
| current_tag = get_commit_tag(commits[0], tags) if commits else None | |
| if during_version_bump and rules.merge_prereleases and not commits: | |
| current_tag = None | |
| else: | |
| # Check if the latest commit is not tagged | |
| current_tag = get_commit_tag(commits[0], tags) |
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 think it would be or not commits, in which case I find it more readable as is.
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, thanks
commitizen/commands/bump.py
Outdated
| "incremental": True, | ||
| "dry_run": dry_run, | ||
| "during_version_bump": self.arguments["prerelease"] | ||
| is None, # We let the changelog implementation know that we want to replace prereleases while staying incremental AND the new tag does not exist already |
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.
The comment is a bit unclear to me.
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.
Fair. Was more of a note to self. I changed it to something more meaningful.
commitizen/changelog_formats/base.py
Outdated
| latest_version_index = index | ||
| line = line.strip().lower() | ||
|
|
||
| parsed = self.parse_version_from_title(line) |
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.
Could you rename the variable parsed to something more clear? Such as parsed_version. As well as the parsed in another function get_metadata_from_file.
Thanks!
commitizen/changelog_formats/base.py
Outdated
| if parsed: | ||
| if not self.tag_rules.extract_version( | ||
| GitTag(parsed.tag, "", "") | ||
| ).is_prerelease: |
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.
Combine these if statements with and
| ) as changelog_file: | ||
| return self.get_latest_full_release_from_file(changelog_file) | ||
|
|
||
| def get_latest_full_release_from_file(self, file: IO[Any]) -> IncrementalMergeInfo: |
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 extract this function? I don't see any benefits.
You could put the whole function body under with open block and the logic is still clear.
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 followed the same pattern as with metadata, which also has an interface calling the get function, which in turn calls the get_from_file function. I think both works, but I would prefer to leave it as is to stay consistent.
|
@woile @bearomorphism Thank you for your reviews! |
|
The latest commit should not be chore. I'd suggest you to rebase the commits and adjust the commit messages, so the commit history will be cleaner after merging |
|
I believe there is a way to mock the datetime in your new tests |
|
I remember git commit timestamp can be specified Thanks!🙏 |
646cc6c to
a16ff63
Compare
|
@bearomorphism Thanks again for the review!
I used freezegun to set a fixed time.
I tried that, but it shows a large diff now. Locally, if I do |
|
@zimmermannJakob could you try to rebase to v4-11-0? Thanks. |
a16ff63 to
9cb370d
Compare
9cb370d to
e9f5ba9
Compare
|
@bearomorphism Thank you, the diff looks good now! Anyway, the PR is good to go from my side. |
Description
This PR solves issue #1694.
Checklist
Code Changes
poetry alllocally to ensure this change passes linter check and tests (Some error out on mac. I will investigate if they fail in ci)- [ ] Update the documentation for the changes(I do not think this applies. It works now as documented.)### Documentation Changes- [ ] Runpoetry doclocally to ensure the documentation pages renders correctly- [ ] Check and fix any broken links (internal or external) in the documentationExpected Behavior
See issue #1694.
Steps to Test This Pull Request
poetry build.Additional Context
Known issue: When the changelog only contains pre-releases, the old behavior can still be observed.(Fixed)