Rethinking init of VersionInfo class? #303

tomschr · 2020-10-26T19:22:13Z

Situation

Originally posted by @tomschr in #258 (comment)

From the above discussion, I thought it would be worth to decouple the compare discussion from the initializer discussion. Both are somewhat related, but can live independently.

With a more "advanced" initializer/constructor we get the following benefits:

more "pythonic": it's one, obvious way to get an instance.
avoids the longer function call Version.parse(...).
more readable

With such an (overloaded?) initializer, we could cover the following use cases:

>>> from semver import Version
>>> Version(1)
Version(major=1, minor=0, patch=0, prerelease=None, build=None)
>>> Version(1, "4", b"5")
Version(major=1, minor=4, patch=5, prerelease=None, build=None)
>>> Version(1, patch=2)
Version(major=1, minor=0, patch=2, prerelease=None, build=None)
>>> Version("1.2.3")
Version(major=1, minor=2, patch=3, prerelease=None, build=None)
>>> Version(b"1.2.3")
Version(major=1, minor=2, patch=3, prerelease=None, build=None)
>>> v = Version(b"2.3.4")
>>> Version(v)
Version(major=2, minor=3, patch=4, prerelease=None, build=None)
>>> t = (1, 2, 3)
>>> Version(*t)                                             
Version(major=1, minor=2, patch=3, prerelease=None, build=None)
>>> d = {'major': 3, 'minor': 4, 'patch': 5,  'prerelease': 'pre.2', 'build': 'build.4'}
>>> Version(**d)
Version(major=3, minor=4, patch=5, prerelease='pre.2', build='build.4')

Discussions and Possible Solutions

To implement a somewhat more advanced constructor/initializer, we have these options:

Program it manually with isinstance and if...else constructs
Use the typing.overload function (suggested by @tlaferriere)
Use functools.singledispatch

However, all three comes at a cost or an issue:

Maybe not impossible, but the code would look probably ugly.
"The @overload decorator is purely for type hints, you can only specify one function body and it has to distinguish the different types using isinstance.` (Originally posted by @tlaferriere in Consider keeping compare module level function #258 (comment))
Is not possible with an __init__ method. The singledispatch works only for functions(!), not methods. For methods we would need functools.singledispatchmethod which is only available from Python >= 3.8.

Another idea goes into a completely different direction. Maybe we shouldn't change the Version class much, but offer a much shorter variant: semver.v.

from functools import singledispatch

# ...
@singledispatch
def ver(major, minor=0, patch=0, prerelease=None, build=None) -> "Version":
    return Version(major, minor, patch, prerelease, build)
    
@ver.register(bytes)
@ver.register(str)
def _(ver: str) -> "Version":
    if isinstance(ver, bytes):
       ver = str(ver, "utf-8")
    if "." in ver:
        return Version.parse(ver)
    return Version(int(ver))

@ver.register(Version)
def _(ver: "Version") -> "Version":
    return ver

Which means, we could just use semver.v as a much shorter variant:

>>> import semver
>>> semver.v("1.2.3")
Version(major=1, minor=2, patch=3, prerelease=None, build=None)

One drawback could be that v is quite short. Maybe too short? Especially if you import it with from semver import v it could be easily overwritten by other, local variables.
That could be a bit avoided to use capital V or ver. Or we use the much longer name semver.version.

Thoughts? Any other ideas? Would that be worth the effort?

@tlaferriere @gsakkis @ppkt

The text was updated successfully, but these errors were encountered:

tomschr · 2020-11-05T23:27:12Z

I found another solution which doesn't need overloading. The previous signature of __init__ was this:

    def __init__(
        major: SupportsInt,
        minor: SupportsInt = 0,
        patch: SupportsInt = 0,
        prerelease: Union[String, int] = None,
        build: Union[String, int] = None,
        )

This changed now to this:

    def __init__(self, *args: Tuple, **kwargs: Dict):
        # print(f">> args={args} {len(args)}, kwargs={kwargs} {len(kwargs)}")
        verlist = [None, None, None, None, None]

        if not args and not kwargs:
            raise ValueError("No argument given.")

        if len(args) > 5:
            raise ValueError("You cannot pass more than 5 arguments to Version")

        if args and "." in str(args[0]):
            # we have a version string as first argument
            cls = self.__class__
            v = cls._parse(args[0])
            self._major = int(v["major"])
            self._minor = int(v["minor"])
            self._patch = int(v["patch"])
            self._prerelease = v["prerelease"]
            self._build = v["build"]
            return

        for index, item in enumerate(args):
            verlist[index] = args[index]

        # Build a dictionary of the arguments except prerelease and build
        try:
            version_parts = {
                # Prefer kwargs over args
                "major": int(kwargs.pop("major", 0) or verlist[0] or 0),
                "minor": int(kwargs.pop("minor", 0) or verlist[1] or 0),
                "patch": int(kwargs.pop("patch", 0) or verlist[2] or 0)
            }
        except ValueError:
            raise ValueError("Expected integer or integer string for major, "
                             "minor, or patch")

        prerelease = kwargs.pop("prerelease", None)
        build = kwargs.pop("build", None)
        # Check, if kwargs contains other unknown keys:
        if kwargs:
            raise ValueError("Unknown key passed in {n}: {k}".format(
                n=self.__class__.__name__,
                k=", ".join([f"{k}={v}"
                             for k, v in kwargs.items()])))

        for name, value in version_parts.items():
            if value < 0:
                raise ValueError(
                    "{!r} is negative. A version can only be positive.".format(name)
                )

        self._major = version_parts["major"]
        self._minor = version_parts["minor"]
        self._patch = version_parts["patch"]
        self._prerelease = None if prerelease is None else str(prerelease)
        self._build = None if build is None else str(build)

This allows some interesting variations:

>>> import semver
>>> semver.Version(1, 2, 3)
Version(major=1, minor=2, patch=3, prerelease=None, build=None)
>>> semver.Version("1.2.3")
Version(major=1, minor=2, patch=3, prerelease=None, build=None)
>>> semver.Version(1, minor="2", patch=3)
Version(major=1, minor=2, patch=3, prerelease=None, build=None)

You can also use positional arguments and keyword arguments to "overwrite" one or the other:

>>> semver.Version(1, major="2", patch=3)
Version(major=2, minor=0, patch=3, prerelease=None, build=None)

Probably the code can be improved, but it's just a proof-of-concept to show this idea. Additionally, it avoids the complicated overload code. What do you think?

* Allow different variants to call Version * Adapt the documentation and README * Adapt and amend tests * Add changelog entries TODO: Fix typing errors

tomschr · 2020-11-06T20:44:14Z

If the above PR got accepted, the Version.parse would become obsolete. In that case, I would suggest to deprecate it.

* Allow different variants to call Version * Adapt the documentation and README * Adapt and amend tests * Add changelog entries TODO: Fix typing errors

* Allow different variants to call Version * Adapt the documentation and README * Adapt and amend tests * Add changelog entries * Introduce a (private) _ensure_str class method

* Allow different variants to call Version * Adapt the documentation and README * Adapt and amend tests * Add changelog entries * Add function "remove_noqa" in conf.py to remove any "# noqa" lines for flake8 issues like overly long lines * Introduce a (private) _ensure_str class method

tomschr added Release_3.x.y Only for the major release 3 Enhancement Not a bug, but increases or improves in value, quality, desirability, or attractiveness labels Oct 26, 2020

tomschr added this to To do in Release 3 via automation Oct 26, 2020

tomschr added the Question Unclear or open issue subject for debate label Oct 26, 2020

tomschr mentioned this issue Oct 30, 2020

Rename VersionInfo to Version #305

Closed

tomschr moved this from To do to In progress in Release 3 Nov 6, 2020

tomschr linked a pull request Nov 10, 2020 that will close this issue

Fix #303: Fix Version.__init__ method #317

Open

tomschr mentioned this issue Nov 11, 2020

Create semver 3.0.0-dev3 #315

Closed

4 tasks

tomschr mentioned this issue Aug 18, 2021

Consider keeping compare module level function #258

Closed

tomschr added the Design Ideas, suggestions, musings about design questions label Nov 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rethinking init of VersionInfo class? #303

Rethinking init of VersionInfo class? #303

tomschr commented Oct 26, 2020 •

edited

tomschr commented Nov 5, 2020 •

edited

tomschr commented Nov 6, 2020 •

edited

Rethinking __init__ of VersionInfo class? #303

Rethinking __init__ of VersionInfo class? #303

Comments

tomschr commented Oct 26, 2020 • edited

Situation

Discussions and Possible Solutions

tomschr commented Nov 5, 2020 • edited

tomschr commented Nov 6, 2020 • edited

Rethinking init of VersionInfo class? #303

Rethinking init of VersionInfo class? #303

tomschr commented Oct 26, 2020 •

edited

tomschr commented Nov 5, 2020 •

edited

tomschr commented Nov 6, 2020 •

edited