Skip to content
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

pattern match on base version in Version constructor #41

Merged
merged 5 commits into from
Feb 17, 2022
Merged

pattern match on base version in Version constructor #41

merged 5 commits into from
Feb 17, 2022

Conversation

marnikow
Copy link
Contributor

When creating a new Version dunamai treats everything as the base. I added pattern matching to the constructor so that more information is extracted from the base string.

Comment on lines 321 to 322
:param pattern: Regular expression matched against the base.
Refer to `from_any_vcs` for more info.
Copy link
Owner

Choose a reason for hiding this comment

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

Could you explain the use case for this? I didn't really think anyone would need to turn a version string back into a Version instance.

If we do add it, I'd probably prefer it to be a separate function, like def parse_version(version: str) -> Version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's use full when you want to modify a version that is gathered from importlib.metadata. Since dunamai just grabs the string, everything is put inside base .

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, that's a great point.

Okay, let's separate this from the constructor and put it as Version.parse(cls, version: str, pattern: str = _VERSION_PATTERN) -> "Version". That way, the constructor stays simple and predictable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've pushed an possible implementation. Version.parse allows a version as well so that you can use the function as follows: dunamai.Version.parse(dunamai.get_version("dunamai", third_choice=dunamai.Version.from_any_vcs))

Copy link
Owner

Choose a reason for hiding this comment

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

To keep parse() simpler, I think I'll just add a parser argument to get_version() after the merge. parser=Version would keep the existing behavior, and parser=Version.parse would enable the new behavior.

dunamai/__init__.py Outdated Show resolved Hide resolved
dunamai/__init__.py Outdated Show resolved Hide resolved
tests/unit/test_dunamai.py Outdated Show resolved Hide resolved
@mtkennerly mtkennerly added the enhancement New feature or request label Feb 17, 2022
@mtkennerly mtkennerly merged commit 3742201 into mtkennerly:master Feb 17, 2022
mtkennerly added a commit that referenced this pull request Feb 17, 2022
@mtkennerly mtkennerly added this to the v1.9.0 milestone Feb 17, 2022
@marnikow marnikow deleted the extended_version branch February 18, 2022 22:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants