Thanks @OidaTiftla for your contribution! And sorry for the delay.

It looks good. My only concern is the repetition of the content between the class variables _ REGEX and _REGEX_ALLOW_SHORT . Both share a lot of similar strings. As far as I can see, the only distinction is that the patch part is optional, right?

If feasible, could be combine these class variables somehow?

I looked a bit closer and I see the following issues:

1. As mentioned before, the two class variables violate the DRY principle.
2. The variable name allowShorterVersion is camel case style. This is Python, we should avoid that. 😉
3. The docstring misses an explanation of the second variable variable of the parse() method.

Apart from the technical issue, there is a fundamental question. Should we deviate from the semver spec? Should we allow parsing of incomplete version strings?
I'm not sure as we already have this documented here: https://python-semver.readthedocs.io/en/latest/advanced/deal-with-invalid-versions.html

Have you seen this?

IF we want to go this route, we should get rid of the two class variables and define a placeholder for the patch part like this:

_REGEX = (
    r"""
        ^
        (?P<major>0|[1-9]\d*)
        (?:
            \.
            (?P<minor>0|[1-9]\d*)
            (?:
                \.
                (?P<patch>0|[1-9]\d*)
            ){}
        )
        (?:-(?P<prerelease>
            (?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)
            (?:\.(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*
        ))?
        (?:\+(?P<build>
            [0-9a-zA-Z-]+
            (?:\.[0-9a-zA-Z-]+)*
        ))?
        $
    """
)

In the parse() method, we need to decide, if we want the optional patch part or not. I have something like this in my mind:

def parse((cls, version: String, optional_patch: bool = False) -> "Version":
    regex = re.compile(cls._REGEX.format("?" if optional_patch else ""),  re.VERBOSE)
    match = regex.match(version)
    if match is None:
         raise ValueError(f"{version} is not valid SemVer string")

     matched_version_parts: Dict[str, Any] = match.groupdict()
     if not matched_version_parts['minor']:
          matched_version_parts['minor'] = 0
     if not matched_version_parts['patch']:
          matched_version_parts['patch'] = 0

     return cls(**matched_version_parts)

This is just a rough draft. I'm not sure if this would be useful as we will loose the re.compile optimization.

Thanks @OidaTiftla for your contribution! And sorry for the delay.

Everything okay. It's open source, so don't stress.

It looks good. My only concern is the repetition of the content between the class variables _ REGEX and _REGEX_ALLOW_SHORT . Both share a lot of similar strings. As far as I can see, the only distinction is that the patch part is optional, right?

Yes, patch and minor is optional.

1. As mentioned before, the two class variables violate the DRY principle.

I fully support this 👍, the format string solution is a very nice idea.

1. The variable name allowShorterVersion is camel case style. This is Python, we should avoid that. 😉

Ah thanks, I'll fix this.

1. The docstring misses an explanation of the second variable variable of the parse() method.

Ok, I'll add it.

Apart from the technical issue, there is a fundamental question. Should we deviate from the semver spec? Should we allow parsing of incomplete version strings? I'm not sure as we already have this documented here: https://python-semver.readthedocs.io/en/latest/advanced/deal-with-invalid-versions.html

Have you seen this?

I was reading though the docs but didn't found that part before trying to adjust the code to fit my needs. So thanks for the link. What does not make me happy about this workaround is, that I have to do the parsing by my own and cannot use the library for that part. So I would copy the regex from the source code of python_semver.

My opinion to the fundamental question: If the method is used with the default parameters it will still throw an error if it is not a valid semver. So I think it would be okay, but that's something you or you with the other maintainers must decide if you want that optional parameter to deviate from the spec. Users would not need the workaround from above and can still benefit from that library. For me it would be totally fine, if you close this PR and I use my fork in my current project.

This is just a rough draft. I'm not sure if this would be useful as we will loose the re.compile optimization.

I've fixed the DRY violation a bit differently, you may have a look at it and report if this would keep the re.compile optimisation.

If you do not want to deviate from the semver spec it's also ok for me, you can simply close this PR. I will then stick to my fork for my current project, that's also fine. 😄

If you decide to continue this PR, I can also add some tests for the optional parsing feature.

@OidaTiftla Thanks for your contribution! 👍

Thanks @OidaTiftla for your contribution! And sorry for the delay.

Everything okay. It's open source, so don't stress.

Haha, yes, you're right.

1. As mentioned before, the two class variables violate the DRY principle.

I fully support this +1, the format string solution is a very nice idea.

Thanks. I would even go further and be explicit. In my example, I wasn't and that could be improved with a named placeholder.

Apart from the technical issue, there is a fundamental question. Should we deviate from the semver spec? Should we allow parsing of incomplete version strings? I'm not sure as we already have this documented here: https://python-semver.readthedocs.io/en/latest/advanced/deal-with-invalid-versions.html
Have you seen this?

I was reading though the docs but didn't found that part before trying to adjust the code to fit my needs. So thanks for the link.

You're welcome. 🙂

What does not make me happy about this workaround is, that I have to do the parsing by my own and cannot use the library for that part. So I would copy the regex from the source code of python_semver.

Not sure if I understand it correctly, but why copying the regex? Wouldn't that be something for creating a subclass from Version? In the derived class you can access the regex from the base class.

My opinion to the fundamental question: If the method is used with the default parameters it will still throw an error if it is not a valid semver. So I think it would be okay, but that's something you or you with the other maintainers must decide if you want that optional parameter to deviate from the spec. Users would not need the workaround from above and can still benefit from that library. For me it would be totally fine, if you close this PR and I use my fork in my current project.

Thanks for your thoughts. 👍

I think, I would be okay if a) the default behavior isn't changed, b) it's clearly documented in the docstring, and c) all tests are green. You already did a and b. Look at c, there are some overly lone lines, but nothing serious.

What I would like to avoid is to blow up the library to an "all-purpose-parse-whatever-shitty-version-string-you have". 😉 There is

This is just a rough draft. I'm not sure if this would be useful as we will loose the re.compile optimization.

I've fixed the DRY violation a bit differently, you may have a look at it and report if this would keep the re.compile optimisation.

I will come up with an additional review.

If you do not want to deviate from the semver spec it's also ok for me, you can simply close this PR. I will then stick to my fork for my current project, that's also fine.

I'm happily integrate your change. Just a few minor things that I will come up in a few seconds.

Thanks for your feedback @tomschr! 👍

What does not make me happy about this workaround is, that I have to do the parsing by my own and cannot use the library for that part. So I would copy the regex from the source code of python_semver.

Not sure if I understand it correctly, but why copying the regex? Wouldn't that be something for creating a subclass from Version? In the derived class you can access the regex from the base class.

Yes you're right, a derived class would have access to it, but I was not explicit enough, sorry for that. My statement of copy the regex will not solve the problem, to parse version strings with optional patch and minor version parts. One must copy the regex from python_semver and adjust it. Hope I could clarify my thought a bit.

I think, I would be okay if a) the default behavior isn't changed, b) it's clearly documented in the docstring, and c) all tests are green. You already did a and b. Look at c, there are some overly lone lines, but nothing serious.

Ok. I'll look into it.

What I would like to avoid is to blow up the library to an "all-purpose-parse-whatever-shitty-version-string-you have". 😉 There is

Yes, I'm totally on your site. And you as maintainer should always ask for the necessity of changes and keep the library clean 👍

@tomschr can you please trigger the workflows again? So I can see if I could fix everything. Thanks in advance.

Locally in my environment there were not more errors when running tox --skip-missing-interpreters

And are the added tests enough or should there be more unit tests added?

Not sure if I understand it correctly, but why copying the regex? Wouldn't that be something for creating a subclass from Version? In the derived class you can access the regex from the base class.

Yes you're right, a derived class would have access to it, but I was not explicit enough, sorry for that. My statement of copy the regex will not solve the problem, to parse version strings with optional patch and minor ve

Ahh, right! Thanks for the clarification!

can you please trigger the workflows again? So I can see if I could fix everything. Thanks in advance.

Done. All green! 😉

Locally in my environment there were not more errors when running tox --skip-missing-interpreters

That's fine. Just in case: you can use pyenv if you need different versions of the Python interpreters. Quite handy!

And are the added tests enough or should there be more unit tests added?

They are fine. 👍

I realized, I haven't looked at some parts in the documentation/changelog. I would like to add some small additions. If that's done, I'll merge them. Promised! 🙂

@tomschr thanks for your feedback.

Locally in my environment there were not more errors when running tox --skip-missing-interpreters

That's fine. Just in case: you can use pyenv if you need different versions of the Python interpreters. Quite handy!

Ah thank you for this, this would help 👍

I realized, I haven't looked at some parts in the documentation/changelog. I would like to add some small additions. If that's done, I'll merge them. Promised! 🙂

It was my fault to read the CONTRIBUTION.rst that late. I happily integrate your suggestions, that's no problem. You did the work, thanks for your help 😄

@OidaTiftla Many thanks for your contributions! 👍

It was my fault to read the CONTRIBUTION.rst that late. I happily integrate your suggestions, that's no problem

No problem. 🙂

If you have further ideas or suggestions I would happily discuss it with you. 🙂 There are also some open issues that would need some love and dedication. Also I'm working on getting out a 3.0.0 release. Not sure when this will be ready.