Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork295
feat: Introduce tag_regex option with smart default#692
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecovbot commentedMar 23, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@## master #692 +/- ##==========================================+ Coverage 97.35% 98.04% +0.69%========================================== Files 42 41 -1 Lines 2041 1742 -299 ==========================================- Hits 1987 1708 -279+ Misses 54 34 -20
Flags with carried forward coverage won't be shown.Click here to find out more.
☔ View full report in Codecov by Sentry. |
We've been using this default already in `normalize_tag`,but setting this value in the settings dict is cleaner.
config_path and changelog_path rely on the modified CWD provided bytmp_commitizen_project, so they should use this fixture.
Closescommitizen-tools#519CLI flag name: --tag-regexHeavily inspired bycommitizen-tools#537, but extendsit with a smart default value to exclude non-release tags. This wassuggested incommitizen-tools#519 (comment)
It looks like these changes need some reworking because they partially duplicate what was done in3a1af6a. Would you merge this if I rework it? It didn't get a review before. |
| VersionProtocol = Any | ||
| def tag_from_version( |
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_to_tag looks more straightforward to me
| deftag_from_version( | |
| defveresion_to_tag( |
| @@ -163,7 +164,9 @@ def get_tags(dateformat: str = "%Y-%m-%d") -> List[GitTag]: | |||
| forlineinc.out.split("\n")[:-1] | |||
| ] | |||
| returngit_tags | |||
| filtered_git_tags= [tfortingit_tagsifpattern.fullmatch(t.name)] | |||
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.
maybe we can use git command to filter . but it's not requried.
Hi@robertschweizer sorry for the late reply. I briefly walk through it. I like the idea and will definitely take a look after reworked 🙂 |
Sorry, I don't have much time at the moment. I started rebasing the first commit and opened#775 to get that merged. To continue here (not sure if I will find the time myself), it's important to consider:
|
Looks good, hopefully we can merge it soon |
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 to go the right way.
I added a few suggestions.
| self.tag_format = args.get("tag_format") or self.config.settings.get( | ||
| "tag_format" | ||
| self.tag_format: str = args.get("tag_format") or self.config.settings.get( | ||
| "tag_format", DEFAULT_SETTINGS["tag_format"] |
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.
You don't need to inject default there, default are already loaded and overwritten by actual settings
| Type: `str` | ||
| Default: Based on `tag_format` |
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.
| Default:Based on`tag_format` | |
| Default:Computed from`tag_format` |
or
| Default:Based on`tag_format` | |
| Default:Extrapolated from`tag_format` |
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.
Not related to this PR, can you remove this ?
| @@ -140,7 +141,7 @@ def get_filenames_in_commit(git_reference: str = ""): | |||
| raiseGitCommandError(c.err) | |||
| defget_tags(dateformat:str="%Y-%m-%d")->List[GitTag]: | |||
| defget_tags(dateformat:str="%Y-%m-%d",*,pattern:re.Pattern)->List[GitTag]: | |||
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.
Make the pattern filtering optional:
| defget_tags(dateformat:str="%Y-%m-%d",*,pattern:re.Pattern)->List[GitTag]: | |
| defget_tags(dateformat:str="%Y-%m-%d",*,pattern:re.Pattern|None=None)->List[GitTag]: |
| from commitizen.tags import make_tag_pattern, tag_from_version | ||
| TAG_FORMATS: Dict[str, Dict[str, list]] = { |
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.
As a failsafe, add a case for the minimal$version that should always work:
| TAG_FORMATS:Dict[str,Dict[str,list]]= { | |
| TAG_FORMATS:Dict[str,Dict[str,list]]= { | |
| "$version": { | |
| "tags_per_version": [ | |
| ("1.2.3","1.2.3"), | |
| ("1.2.3a2","1.2.3a2"), | |
| ("1.2.3b2","1.2.3b2"), | |
| ("1.2.3+1.0.0","1.2.3+1.0.0"), | |
| ], | |
| "invalid_tags": ["v1.2.3","unknown-tag","1-2-3","ver1.0.0"] | |
| }, |
AlexeySanko commentedMay 24, 2024
Looks as great feature for monorepo. Any update on it? |
@robertschweizer do you think you could rebase? thanks! |
Sorry, I won't have time to look into this in the next weeks. |
I am wondering if#1297 is not providing somehow the same feature, but I might be wrong. |
Description
Closes#519
CLI flag name: --tag-regex
Heavily inspired by#537, but extends it with a smart default value to exclude non-release tags. This was suggested in#519 (comment)
I separated commits to make review easier.
Checklist
./scripts/formatand./scripts/testlocally to ensure this change passes linter check and testExpected behavior
Steps to Test This Pull Request
Additional context