Search code, repositories, users, issues, pull requests...

Copy link

Contributor Author

grahamhar commented Mar 2, 2024

I have taken a stab at the test case.

Lowaiz

Lowaiz reviewed

Mar 22, 2024

commitizen/commands/changelog.py

Comment on lines 173 to 185

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

)

Copy link

Contributor

@Lowaiz Lowaiz Mar 22, 2024 •

edited

Loading

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.

Copy link

Contributor Author

@grahamhar grahamhar Mar 26, 2024

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.

Copy link

Contributor

@Lowaiz Lowaiz Mar 26, 2024

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.

Copy link

Contributor Author

@grahamhar grahamhar Mar 26, 2024

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.

Copy link

Contributor Author

@grahamhar grahamhar Mar 26, 2024

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.

Copy link

Member

@Lee-W Lee-W Apr 2, 2024

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?

Copy link

Contributor Author

@grahamhar grahamhar Apr 2, 2024

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

Copy link

Member

@Lee-W Lee-W Apr 2, 2024 •

edited

Loading

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.

Copy link

Contributor Author

@grahamhar grahamhar Apr 7, 2024

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.

Copy link

Member

@Lee-W Lee-W Apr 8, 2024

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

@grahamhar grahamhar marked this pull request as draft

March 26, 2024 13:26

@grahamhar grahamhar force-pushed the regex-tags branch from a7cea66 to fcfd4dc Compare

March 26, 2024 18:25

@Lee-W Lee-W added pr-status: wait-for-response and removed pr-status: wait-for-review labels

Mar 30, 2024

@grahamhar grahamhar force-pushed the regex-tags branch from fcfd4dc to d6b0745 Compare

April 1, 2024 17:43

@github-actions github-actions bot added the pr-status: wait-for-review label

Apr 1, 2024

@grahamhar grahamhar force-pushed the regex-tags branch 8 times, most recently from 0cc5cef to 7d74d80 Compare

April 7, 2024 13:42

@grahamhar grahamhar marked this pull request as ready for review

April 7, 2024 13:50

@Lee-W Lee-W removed the pr-status: wait-for-response label

Apr 8, 2024

noirbizarre

noirbizarre requested changes

Apr 17, 2024

Copy link

Member

@noirbizarre noirbizarre left a 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

Comment on lines 34 to 47

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+)?",

}

Copy link

Member

@noirbizarre noirbizarre Apr 17, 2024

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 and tag_format_regexps_for

commitizen/providers/scm_provider.py Outdated

Comment on lines 34 to 39

"${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+)?",

Copy link

Member

@noirbizarre noirbizarre Apr 17, 2024

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+)?",

Copy link

Member

@noirbizarre noirbizarre Apr 17, 2024

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,

Copy link

Contributor Author

grahamhar commented Apr 20, 2024

Seems good but:

TAG_FORMAT_REGEXS should be factorized and declared once

the new ${} syntax should be documented

I think I have implemented the suggestions

@Lee-W Lee-W requested a review from noirbizarre

April 21, 2024 06:53

Lee-W

Lee-W reviewed

Apr 21, 2024

commitizen/changelog_formats/asciidoc.py Outdated Show resolved Hide resolved

commitizen/changelog_formats/markdown.py Outdated Show resolved Hide resolved

commitizen/providers/scm_provider.py Show resolved Hide resolved

commitizen/changelog_formats/restructuredtext.py Show resolved Hide resolved

commitizen/changelog_formats/textile.py Outdated Show resolved Hide resolved

commitizen/defaults.py Outdated

bump_message = "bump: version $current_version → $new_version"

def get_tag_regexes(version_regex: str) -> dict[str | Any, str | Any]:

Copy link

Member

@Lee-W Lee-W Apr 21, 2024

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?

Copy link

Contributor Author

@grahamhar grahamhar Apr 27, 2024

Choose a reason for hiding this comment

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

Fixed

@Lee-W Lee-W added the pr-status: wait-for-modification label

Apr 21, 2024

@Lee-W Lee-W assigned noirbizarre and woile and unassigned Lee-W

May 20, 2024

@Lee-W

Copy link

Member

Lee-W commented May 20, 2024

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 poetry run python scripts/gen_cli_help_screenshots.py

@Lee-W Lee-W added the pr-status: wait-for-modification label

May 20, 2024

woile

woile reviewed

May 21, 2024

docs/tutorials/monorepo_guidance.md Show resolved Hide resolved

@Lee-W Lee-W unassigned woile

May 22, 2024

Copy link

Contributor Author

grahamhar commented May 22, 2024

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 🙂

No worries on the time taken, I will address all the points but might take me a week due to other commitments

@grahamhar grahamhar force-pushed the regex-tags branch from 174d809 to 7400023 Compare

May 29, 2024 19:33

Copy link

Contributor Author

grahamhar commented May 29, 2024

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 poetry run python scripts/gen_cli_help_screenshots.py

Hi @Lee-W,

I have attempted to address the comments, please validate when you have time.

When attempting to run the gen_cli_help_screenshots.py script requested above I just get this output which appears to just be given usage errors of cz

image

I'm not sure if you are already aware, but with the push hook in place I got an error for a commit not in my PR

commit validation: failed!
please enter a commit message in the commitizen format.
commit "37522866e4788deb12b2ef1c426662400b0ebac8": "docs(cli/screenshots) update CLI screenshots"
pattern: (?s)(build|ci|docs|feat|fix|perf|refactor|style|test|chore|revert|bump)(\(\S+\))?!?:( [^\n\r]+)((\n\n.*)|(\s*))?$

@Lee-W

Copy link

Member

Lee-W commented May 30, 2024

When attempting to run the gen_cli_help_screenshots.py script requested above I just get this output which appears to just be given usage errors of cz

The screenshot doesn't look like an error to me 🤔 so basically, this is a command we use to generate the latest cz help message. so if the screenshots are generated, I think we are good 🙂

I'm not sure if you are already aware, but with the push hook in place I got an error for a commit not in my PR

Even if you try with rebase? There was one commit with an ill-formatted message. But our hook should only check the commits in this branch, I think 🤔 It Looks like the CI is passing now. Is this still an issue?

@Lee-W Lee-W added pr-status: wait-for-response and removed pr-status: wait-for-modification labels

May 30, 2024

Copy link

Contributor Author

grahamhar commented May 31, 2024

When attempting to run the gen_cli_help_screenshots.py script requested above I just get this output which appears to just be given usage errors of cz

The screenshot doesn't look like an error to me 🤔 so basically, this is a command we use to generate the latest cz help message. so if the screenshots are generated, I think we are good 🙂

I'm not sure if you are already aware, but with the push hook in place I got an error for a commit not in my PR

Even if you try with rebase? There was one commit with an ill-formatted message. But our hook should only check the commits in this branch, I think 🤔 It Looks like the CI is passing now. Is this still an issue?

Maybe my PR didn't need any changes to the generated images, possibly just confusion on my side.

I have just made sure my fork is updated and as I have no changes I just tried running the hook

 pre-commit run --hook-stage pre-push 
Check hooks apply to the repository.......................(no files to check)Skipped
Check for useless excludes................................(no files to check)Skipped
check vcs permalinks......................................(no files to check)Skipped
fix end of files..........................................(no files to check)Skipped
trim trailing whitespace..................................(no files to check)Skipped
debug statements (python).................................(no files to check)Skipped
don't commit to branch........................................................Passed
check for merge conflicts.................................(no files to check)Skipped
check toml................................................(no files to check)Skipped
check yaml................................................(no files to check)Skipped
detect private key........................................(no files to check)Skipped
blacken-docs..............................................(no files to check)Skipped
Run codespell to check for common misspellings in files...(no files to check)Skipped
commitizen check branch.......................................................Failed
- hook id: commitizen-branch
- exit code: 23
fatal: ambiguous argument 'origin/HEAD..HEAD': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
format....................................................(no files to check)Skipped
linter and test...........................................(no files to check)Skipped

Doing some further investigation my fork doesn't have origin/HEAD set. After running

git remote set-head origin master

The hook now works :)

@Lee-W

Copy link

Member

Lee-W commented May 31, 2024

The hook now works :)

Niiiiiiice! I will probably not be around for half a month, but I will try to take a look after I'm back from traveling. Thanks for actively helping out.

@Lee-W Lee-W removed the pr-status: wait-for-response label

Jun 27, 2024

@noirbizarre

Copy link

Member

noirbizarre commented Aug 20, 2024

Sorry for the looong absence.
I'll take the time to do a final review by the end of the week (as this one is a big one, and I need to reread and remember all what have been said)

@woile

Copy link

Member

woile commented Sep 12, 2024

This could potentially fix #1149 right?

woile

woile approved these changes

Sep 16, 2024

Copy link

Member

@woile woile left a comment

Choose a reason for hiding this comment

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

LGTM @Lee-W @noirbizarre any thoughts?

grahamhar and others added 6 commits

September 16, 2024 14:16


 fix(changelog): handle custom tag_format in changelog generation

13116d5

When the tag_format does not follow the allowed schemas patterns then changlog generation fails.


 test(changelog): handle custom tag_format in changelog generation

1d493aa


 fix(changelog): Handle tag format without version pattern

2e1c553


 fix(changelog): Factorized TAG_FORMAT_REGEXES

97eb90c