-
-
Notifications
You must be signed in to change notification settings - Fork 266
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
feat: Introduce tag_regex option with smart default #692
base: master
Are you sure you want to change the base?
feat: Introduce tag_regex option with smart default #692
Conversation
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 by tmp_commitizen_project, so they should use this fixture.
Closes commitizen-tools#519 CLI flag name: --tag-regex Heavily inspired by commitizen-tools#537, but extends it with a smart default value to exclude non-release tags. This was suggested in commitizen-tools#519 (comment)
8eea9ea
to
e58e56f
Compare
It looks like these changes need some reworking because they partially duplicate what was done in 3a1af6a. 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
def tag_from_version( | |
def veresion_to_tag( |
@@ -163,7 +164,9 @@ def get_tags(dateformat: str = "%Y-%m-%d") -> List[GitTag]: | |||
for line in c.out.split("\n")[:-1] | |||
] | |||
|
|||
return git_tags | |||
filtered_git_tags = [t for t in git_tags if pattern.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 = ""): | |||
raise GitCommandError(c.err) | |||
|
|||
|
|||
def get_tags(dateformat: str = "%Y-%m-%d") -> List[GitTag]: | |||
def get_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:
def get_tags(dateformat: str = "%Y-%m-%d", *, pattern: re.Pattern) -> List[GitTag]: | |
def get_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"] | |
}, |
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. |
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/format
and./scripts/test
locally to ensure this change passes linter check and testExpected behavior
Steps to Test This Pull Request
Additional context