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(changelog): handle custom tag_format in changelog generation #995
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #995 +/- ##
==========================================
+ Coverage 97.33% 97.59% +0.25%
==========================================
Files 42 55 +13
Lines 2104 2536 +432
==========================================
+ Hits 2048 2475 +427
- 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. |
You can add tests similar to this: |
I have taken a stab at the test case. |
commitizen/commands/changelog.py
Outdated
latest_tag_version: str = bump.normalize_tag( | ||
changelog_meta.latest_version, | ||
tag_format=self.tag_format, | ||
scheme=self.scheme, | ||
) | ||
start_rev = self._find_incremental_rev( | ||
strip_local_version(latest_tag_version), tags | ||
strip_local_version(changelog_meta.latest_version), 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.
Hello,
Removing this logic is breaking the search for custom tags (with a tag-format like example-${version}
).
The first bump (with --changelog
option) will have no problem and it will create the CHANGELOG.md
file but any bump afterward will result in an error: commitizen.exceptions.NoRevisionError: No tag found to do an incremental changelog
.
This is due to the _find_incremental_rev
function that look at similarity between a given tag and the tags list. But without the normalization, it calculates similarity between a stripped tag (without any format so like 0.1.1
) and the formatted tag (like example-0.1.0
) resulting in a similarity below threshold.
So, except if you have another edge case justifying this removal, I think it should be kept.
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.
Hi,
Thanks for the feedback, I'll put this to a draft status and try to find a solution that doesn't break the current way of working. My gut feeling is it might need an additional flag maybe something like --strict-tag-matching
what are your thoughts on taking that approach?
I guess the fact you found that error might mean there is a missing test case, I will try adding that first to replicate the error with my changes so I have something to validate against.
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 used your version with a revert on the selected part (here), and it seems to works pretty damn well for me, that's why I was asking for the "why" of the change.
I was able to bump and generate for multiple cz.toml file and multiple tag-format (in a monorepo with 3 sub-packages).
The normalizing part was introduced by that PR, exactly fixing the error I got.
I really think that you just need to keep the normalizing part, and we should be OK.
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.
Hi,
Thanks for clarifying I misunderstood your point.
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 added a test that shows the failure you describe with my approach.
However, when I update my test to then do a second commit/bump then I start to get a failure as the changelog_meta.latest_version
for my scenario where the custom tag values comes after the version number is returned as 0.2.0custom (str) which then causes an Invalid version error from the call to scheme in the bump.normalize_tag function.
I found that the change log is written with tag_format in the title for each section but then parse_version_from_title
uses the parser from the version schema which means that when the tag format has a suffix rather than a prefix the issue occurs.
I will try to investigate further but would value your thoughts.
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.
Is there anything that needs clarification for this one? (I guess not?) or should I start reviewing?
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 just want to check the way I have handled the different possible tag_formats in markdown.py seems OK. If it is I will make the appropriate changes to the other formats taking the same approach. Once I have completed that it will be ready for review
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 don't see any major flaws at first glance, but I will try to take a deeper look this weekend.
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.
Sorry it took a while for me to get this finished off. Ready for review when you have time.
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! I was not able to review it last week. 😞 Let's see whether I can at least check a portion of it this week. 💪
5a9da67
to
21c46b1
Compare
When the tag_format does not follow the allowed schemas patterns then changlog generation fails.
d1d6611
to
0cc5cef
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.
Seems good but:
TAG_FORMAT_REGEXS
should be factorized and declared once- the new
${}
syntax should be documented
commitizen/changelog_formats/base.py
Outdated
TAG_FORMAT_REGEXS = { | ||
"$version": version_regex, | ||
"$major": r"(?P<major>\d+)", | ||
"$minor": r"(?P<minor>\d+)", | ||
"$patch": r"(?P<patch>\d+)", | ||
"$prerelease": r"(?P<prerelease>\w+\d+)?", | ||
"$devrelease": r"(?P<devrelease>\.dev\d+)?", | ||
"${version}": version_regex, | ||
"${major}": r"(?P<major>\d+)", | ||
"${minor}": r"(?P<minor>\d+)", | ||
"${patch}": r"(?P<patch>\d+)", | ||
"${prerelease}": r"(?P<prerelease>\w+\d+)?", | ||
"${devrelease}": r"(?P<devrelease>\.dev\d+)?", | ||
} |
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.
Those are exactly the same as in changelog:get_version_tag
so I suggest to factorize this into a public factory, something like:
TAG_FORMAT_REGEXS = {
"$major": r"(?P<major>\d+)",
"$minor": r"(?P<minor>\d+)",
"$patch": r"(?P<patch>\d+)",
"$prerelease": r"(?P<prerelease>\w+\d+)?",
"$devrelease": r"(?P<devrelease>\.dev\d+)?",
"${version}": version_regex,
"${major}": r"(?P<major>\d+)",
"${minor}": r"(?P<minor>\d+)",
"${patch}": r"(?P<patch>\d+)",
"${prerelease}": r"(?P<prerelease>\w+\d+)?",
"${devrelease}": r"(?P<devrelease>\.dev\d+)?",
}
def tag_format_regexps_for(version: Pattern) -> dict[str, Pattern]:
return {
"$version": version,
"${version}": version,
**TAG_FORMAT_REGEXS
}
This way:
- single source of thrust
- custom format implementations can reuse
TAG_FORMAT_REGEXS
andtag_format_regexps_for
commitizen/providers/scm_provider.py
Outdated
"${version}": r"(?P<version>.+)", | ||
"${major}": r"(?P<major>\d+)", | ||
"${minor}": r"(?P<minor>\d+)", | ||
"${patch}": r"(?P<patch>\d+)", | ||
"${prerelease}": r"(?P<prerelease>\w+\d+)?", | ||
"${devrelease}": r"(?P<devrelease>\.dev\d+)?", |
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.
This is a new accepted syntax.
Documentation and conventional commit message should reflect that
commitizen/providers/scm_provider.py
Outdated
"${minor}": r"(?P<minor>\d+)", | ||
"${patch}": r"(?P<patch>\d+)", | ||
"${prerelease}": r"(?P<prerelease>\w+\d+)?", | ||
"${devrelease}": r"(?P<devrelease>\.dev\d+)?", |
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 similar to the 2 previous factorizable TAG_FORMAT_REGEXS
so I think it should be factorized too,
I think I have implemented the suggestions |
commitizen/defaults.py
Outdated
@@ -133,3 +133,20 @@ class Settings(TypedDict, total=False): | |||
) | |||
change_type_order = ["BREAKING CHANGE", "Feat", "Fix", "Refactor", "Perf"] | |||
bump_message = "bump: version $current_version → $new_version" | |||
|
|||
|
|||
def get_tag_regexes(version_regex: str) -> dict[str | Any, str | Any]: |
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.
When will the key return Any
?
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.
Fixed
Co-authored-by: Wei Lee <weilee.rx@gmail.com>
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.
Sorry for taking so long to review. Most of my comments are just nitpicks. It would be great if we could fix them, but I'm good with it if we cannot. As this is somewhat a larger change, I think it would be better if we can have @woile and @noirbizarre take a look as well 🙂
"${minor}": r"(?P<minor>\d+)", | ||
"${patch}": r"(?P<patch>\d+)", | ||
"${prerelease}": r"(?P<prerelease>\w+\d+)?", | ||
"${devrelease}": r"(?P<devrelease>\.dev\d+)?", |
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.
nitpick: not sure we can deduplicate $.*
and ${.*}
🤔 Non-blocker for this PR
| `$version` | full generated version | | ||
| `$major` | MAJOR increment | | ||
| `$minor` | MINOR increment | | ||
| `$patch` | PATCH increment | | ||
| `$prerelease` | Prerelease (alpha, beta, release candidate) | | ||
| `$devrelease` | Development release | | ||
| `${version}` | full generated version | | ||
| `${major}` | MAJOR increment | | ||
| `${minor}` | MINOR increment | | ||
| `${patch}` | PATCH increment | | ||
| `${prerelease}` | Prerelease (alpha, beta, release candidate) | | ||
| `${devrelease}` | Development 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.
| `$version` | full generated version | | |
| `$major` | MAJOR increment | | |
| `$minor` | MINOR increment | | |
| `$patch` | PATCH increment | | |
| `$prerelease` | Prerelease (alpha, beta, release candidate) | | |
| `$devrelease` | Development release | | |
| `${version}` | full generated version | | |
| `${major}` | MAJOR increment | | |
| `${minor}` | MINOR increment | | |
| `${patch}` | PATCH increment | | |
| `${prerelease}` | Prerelease (alpha, beta, release candidate) | | |
| `${devrelease}` | Development release | | |
| `$version`, `${version}` | full generated version | | |
| `$major`, `${major}` | MAJOR increment | | |
| `$minor`, `${minor}` | MINOR increment | | |
| `$patch`, `${patch}` | PATCH increment | | |
| `$prerelease`, `${prerelease}` | Prerelease (alpha, beta, release candidate) | | |
| `$devrelease`, ${devrelease}` | Development release | |
I think it might make it easier to read.
library-a | ||
.cz.toml | ||
library-b | ||
.cz.toml |
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.
library-a | |
.cz.toml | |
library-b | |
.cz.toml | |
. | |
├── library-b | |
│ └── .cz.toml | |
└── library-z | |
└── .cz.toml |
src | ||
library-b | ||
.cz.toml | ||
library-z | ||
.cz.toml |
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.
src | |
library-b | |
.cz.toml | |
library-z | |
.cz.toml | |
src | |
├── library-b | |
│ └── .cz.toml | |
└── library-z | |
└── .cz.toml |
git.tag("random0.2.0") | ||
testargs = ["cz", "bump", "--changelog", "--yes"] | ||
mocker.patch.object(sys, "argv", testargs) | ||
cli.main() |
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.
cli.main() | |
# bump to 0.2.0custom | |
cli.main() |
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.
Read a few times to understand fully. would be great if we can have comments here 🙂
cli.main() | ||
wait_for_tag() | ||
create_file_and_commit("feat: another new file") | ||
cli.main() |
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.
cli.main() | |
# bump to 0.3.0custom | |
cli.main() |
|
||
|
||
@pytest.mark.parametrize( | ||
"format_with_tags, tag_string, expected, ", |
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.
"format_with_tags, tag_string, expected, ", | |
"format_with_tags, tag_string, expected", |
pytest.param("${version}-example", "1.0.0-example", "1.0.0"), | ||
pytest.param("${version}example", "1.0.0example", "1.0.0"), | ||
pytest.param("example${version}", "example1.0.0", "1.0.0"), | ||
pytest.param("example-${version}", "example-1.0.0", "1.0.0"), | ||
pytest.param("example-${major}-${minor}-${patch}", "example-1-0-0", "1.0.0"), | ||
pytest.param("example-${major}-${minor}", "example-1-0-0", None), | ||
pytest.param( | ||
"${major}-${minor}-${patch}-${prerelease}-example", | ||
"1-0-0-rc1-example", | ||
"1.0.0-rc1", | ||
), | ||
pytest.param( | ||
"${major}-${minor}-${patch}-${prerelease}-example", | ||
"1-0-0-a1-example", | ||
"1.0.0-a1", | ||
), | ||
pytest.param( | ||
"${major}-${minor}-${patch}-${prerelease}${devrelease}-example", | ||
"1-0-0-a1.dev1-example", | ||
"1.0.0-a1.dev1", | ||
), |
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.
feel like we can make it reuseable somewhere 🤔
We probably need to resolve the conflict as well 👀 . We're now using screenshots as part of our doc. you can generate the latest screenshot through |
.cz.toml | ||
``` | ||
|
||
Each component will have its own changelog, commits will need to use scopes so only relevant commits are included in the |
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.
What if you are not using conventional commits and you don't have scopes?
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.
Sorry, I guess I made an assumption here that everyone uses them. Should I just reword this to reflect that assumption?
No worries on the time taken, I will address all the points but might take me a week due to other commitments |
When the tag_format does not follow the allowed schemas patterns then changlog generation fails.
Description
I am working in a mono repo with multiple
.cz.toml
configs (one per component) usingtag_format
to create tags for each component so they can be released independent of each other. When trying to generate the changelog for each component errors are generated see #845.I was unsure how to add a test case for this if you have ideas please let me know and I will be happy to add.
Checklist
./scripts/format
and./scripts/test
locally to ensure this change passes linter check and testExpected behavior
changelogs will now be generated correctly when running
cz bump --changelog
Steps to Test This Pull Request