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

Use case: Introduce VersionInfo.coerce #210

Closed
tomschr opened this issue Dec 9, 2019 · 9 comments
Closed

Use case: Introduce VersionInfo.coerce #210

tomschr opened this issue Dec 9, 2019 · 9 comments
Assignees
Labels
Enhancement Not a bug, but increases or improves in value, quality, desirability, or attractiveness Question Unclear or open issue subject for debate

Comments

@tomschr
Copy link
Member

tomschr commented Dec 9, 2019

Situation

Currently, if you parse a version string into a valid semver version, the version string has to consist of at least the major, minor, and patch parts.

However, in some use cases some parts could be missing, for example the patch part.

Proposed Solution

I propose a classmethod VersionInfo.coerce with the following signature:

@classmethod
def coerce(cls, version):
    """
    Convert an incomplete version string into a semver-compatible VersionInfo
    object

    * Tries to detect a "basic" version string (``major.minor.patch``).
    * If not enough components can be found, missing components are
      set to zero to obtain a valid semver version.

    :param str version: the version string to convert
    :return: a tuple with a :class:`VersionInfo` instance (or ``None``
        if it's not a version) and the rest of the string which doesn't
        belong to a basic version.
    :rtype: tuple(:class:`VersionInfo` | None, str)
    """

I've chosen coerce as it is a common function name in other semver implementations, for example, node-semver.

An example session:

>>> import semver
>>> semver.VersionInfo.coerce("v1.2")
(VersionInfo(major=1, minor=2, patch=0, prerelease=None, build=None), '')
>>> semver.VersionInfo.coerce("3.5rc1")
(VersionInfo(major=3, minor=5, patch=0, prerelease=None, build=None), 'rc1')

Questions:

  • Do you you think such a function could be useful?
  • Should the coerce function accept an integer value (=major)? So coerce(2) would be the same as coerce("2") which would return VersionInfo(major=2, minor=0, patch=0, prerelease=None, build=None).
  • Anything else you miss, would change, ...?

See also

This issue is related to #137

@tomschr tomschr added Question Unclear or open issue subject for debate Enhancement Not a bug, but increases or improves in value, quality, desirability, or attractiveness labels Dec 9, 2019
@tomschr tomschr self-assigned this Dec 9, 2019
tomschr added a commit to tomschr/python-semver that referenced this issue Dec 9, 2019
Signature:
VersionInfo.coerce(cls, version:str) -> tuple
  Convert an incomplete version string into a semver-compatible VersionInfo
  object
  - Tries to detect a "basic" version string (``major.minor.patch``).
  - If not enough components can be found, missing components are
    set to zero to obtain a valid semver version.
tomschr added a commit to tomschr/python-semver that referenced this issue Dec 9, 2019
Signature:
* VersionInfo.coerce(cls, version:str) -> tuple
  Convert an incomplete version string into a semver-compatible VersionInfo
  object
  - Tries to detect a "basic" version string (``major.minor.patch``).
  - If not enough components can be found, missing components are
    set to zero to obtain a valid semver version.

* Update CHANGELOG.rst
@ppkt
Copy link
Member

ppkt commented Dec 10, 2019

To be honest - I'm not sure about this. Generally speaking it's a quite nice feature, but it's for someone who doesn't use semver correctly and I wonder if we should support such "non-semver" behavior.

Parsing versions in non-trivial task and there are a lot of problems with different version formats :)
https://www.python.org/dev/peps/pep-0386/

@tomschr
Copy link
Member Author

tomschr commented Dec 10, 2019

Thanks Karl for your feedback! 👍

I agree, parsing a version can be difficult. However, when I've created this idea, I had one use case in my mind: what does a developer do when he has to deal with a invalid version string that he or she doesn't control? How do we convert such invalid version into a valid semver version?

Currently, we don't have a solution. Sure, the developer could do some "manual massaging" of the version string, but is that a good idea? Isn't that the task for a library (in this specific case: semver)?

With this PR we have an idea to discuss. 😉 I see this coerce function more as a "door opener" that our version string becomes a valid semver version. Usually there are lots of different version strings in the wild, this function tries to at least convert the most common ones like v1, 2.1 etc. to make it easier for our developers.

Additionally, this function focuses on one very narrow definition of a version string: major and optional minor and/or patch. There are no prereleases or build and that's on purpose. If there are further information, this is returned as the second item.

If it turns out that it is actually not needed, I'm absolutely fine with closing this PR.

Hope that clarifies it a bit.

@ppkt
Copy link
Member

ppkt commented Dec 11, 2019

@tomschr I see your point and I didn't mean to sound negative in my previous post - I only wanted to raise my concerns. If we are going to use such "lenient" approach (like in mentioned in #137) it would be a very nice feature. I'm a bit worried, however, that we may run into supporting some different and weird version formats (like mentioned in PEP 386 0.5b3) :)

@tomschr
Copy link
Member Author

tomschr commented Dec 12, 2019

@ppkt I see your point Karol. Don't worry, you didn't sound negative. 😄 👍
Absolutely fine if you raise your concerns, 👍 that's why I've opened this issue for discussion and asked for feedback. 😉

I agree, we shouldn't support "weird" version formats (well, what's "weird" is probably open for discussion). We can't support them all. On the other hand, should we "support" the conversion of some common ones at least (like the mentioned v1 etc.)? Should we help our developers in the conversion to a valid semver? Or should they do it on their own?

My concern is that we leave our developers in the dark when someone has to deal with "invalid" versions. What would be your solution when dealing with such versions?

I'm totally fine if we close this PR, but then we should clearly document the scope of this project and library. Additionally, we should give some recommendations what the developer could do in such a case.

Does this makes sense? 😉

@tomschr
Copy link
Member Author

tomschr commented Dec 12, 2019

@ppkt I just had another idea, similar to my last comment: 💡

If you fear we support "weird" versions when we merge this PR, why not just document how to deal with invalid/weird versions?

For example, we could introduce a new section "Dealing with Invalid Versions" in our documentation where we could add this coerce function as an example (or another, similar function). Give some recommendations, but clearly state that we only support valid semver versions and why invalid versions are out of scope for this library.

Would that be an option?

@scls19fr What do you think? Which one would you prefer? 😉

tomschr added a commit to tomschr/python-semver that referenced this issue Dec 12, 2019
Signature:
* VersionInfo.coerce(cls, version:str) -> tuple
  Convert an incomplete version string into a semver-compatible VersionInfo
  object
  - Tries to detect a "basic" version string (``major.minor.patch``).
  - If not enough components can be found, missing components are
    set to zero to obtain a valid semver version.

* Update CHANGELOG.rst
@tomschr
Copy link
Member Author

tomschr commented Jan 18, 2020

@python-semver/reviewers Any new opinions on this issue?

Just to summarize the two alternative approaches:

a. use the coerce function (maybe with some adaptions) or,
b. don't use it, but document how to deal with invalid version.

@s-celles
Copy link
Member

+1 about documenting

tomschr added a commit to tomschr/python-semver that referenced this issue Jan 19, 2020
* Document how to deal with invalid versions
* Use coerce(version) as an example
@tomschr
Copy link
Member Author

tomschr commented Jan 19, 2020

@scls19fr Ok, let's document this. 👍

I've opened PR #215 and introduced a new section "Dealing with Invalid Versions". There I've used a modified coerce() function as an example.

It's a start, I'm not sure if this is enough. What do you think?

tomschr added a commit to tomschr/python-semver that referenced this issue Jan 19, 2020
* Document how to deal with invalid versions
* Use coerce(version) as an example
* Update CHANGELOG.rst
@s-celles
Copy link
Member

I think that's enough for now.
Thanks @tomschr for your contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Not a bug, but increases or improves in value, quality, desirability, or attractiveness Question Unclear or open issue subject for debate
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants