Skip to content

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Make Version properties change from read-only to read-write? #327

Copy link
Copy link
Open
@tomschr

Description

@tomschr
Issue body actions

Situation

After having invested a lot of time in #317 with __init__() and validating our data, I did some research and here is another of my crazy ideas. 😄 Sorry if the text is a bit longer, but I think it could be worth.

It might simplify __init__() but it would change the way our properties are handled a bit.

According to the section Accessing Parts of a Version Through Names in our documentation, the Version class contains the properties major, minor etc.

However, at the time of writing, these properties can be read only. They do not allow write access:

>>> v = Version.parse("3.4.5-pre.2+build.4")
>>> v.minor = 5
Traceback (most recent call last):
...
AttributeError: attribute 'minor' is readonly

The Idea

I'm wondering if this behavior should be changed. 🤔 Is that still contemporary? I can't exactly remember why we did that. 😉

Maybe our users have a use case which they would like to write something like this:

>>> v.major = 4
>>> v.minor = "10"

Why shouldn't they? Would that make sense?

Of course, the assignment should only be successful if we pass the right type. Invalid values are still rejected and would lead to an exception:

>>> v.major = "x"
Traceback (most recent call last):
...
ValueError: major allows only integers or strings that can be converted to integers.

The Implementation

The idea is to start with some validating functions. We distinguish basically between two types:

  • the main parts consists of major, minor, and patch.
    They must be of type int or str. Furthermore, if it is an integer, the value must be greater than zero.
  • the "other" parts are prerelease and build.
    Here we need to check if it is None (allowed) or a of type int or str (also allowed).

We end up with this:

def validate_mainparts(value: Union[str, int]):
    """
    Validate arguments for major, minor, or patch.

    :param value: the value.
        * Must be of type int, str.
        * If it's an int, value must be value > 0
    :return: Nothing, if everything is okay, otherwise raise exceptions.
    """
    if not isinstance(value, (int, str)):
        raise TypeError(f"The value {value} has wrong type {type(value)}. "
                        "Expected int or str.")
    value = int(value)  # could also raise an exception
    if value < 0:
        raise ValueError(f"The value {value} cannot be negative. "
                         "A version is always positive.")

def validate_otherparts(value: Optional[Union[str, int]]):
    """
    Validate arguments for prerelease and build.

    :param value: the value. Can be None, int, or str.
    :return: Nothing, if everything is okay, otherwise raise TypeError exception.
    """
    if isinstance(value, (int, str, type(None))):
        return
    raise TypeError(f"The value {value} has wrong type {type(value)}. "
                        "Expected int or str.")

Furthermore, we need a decorator/function that creates this property automatically for us. That decorator needs to be called with a name (like major), a callable to check the value (see above), and another callable to convert it to the target type. One implementation could look like:

def typed_property(name: str, validate: Callable, convert: Callable):
    """
    Create properties for version parts.

    :param name: The name of the property.
    :param validate: a function which validates the value that is going to be set.
        Returns nothing if all is good, otherwise raise ValueError or TypeError
        exceptions if the value couldn't be validated.
    :param: convert: a function which converts the input data name to the target
        type.
    :return: the property
    """
    private_name = f"_{name}"

    def fget(self):
        return getattr(self, private_name)

    def fset(self, value):
        validate(value)  # May raise exceptions here
        setattr(self, private_name, convert(value))  # all fine, so set the private name

    return property(
        fget=fget, fset=fset, doc=f"""The {name} part of a version (read and write)."""
    )

To combine it in our class, we need to add it at the class level:

class Version:
    """ ... """
    major = typed_property("major", validate_mainparts, int)
    minor = typed_property("minor", validate_mainparts, int)
    patch = typed_property("patch", validate_mainparts, int)
    prerelease = typed_property("prerelease", validate_otherparts, lambda x: x if x is None else str(x))
    build = typed_property("build", validate_otherparts, lambda x: x if x is None else str(x))

   # ... no other @property...

I see here some benefits:

  • We don't need to repeat code. All is hidden inside typed_property and the validating functions.
  • The initializer __init__ is free from validating the inputs. This is all done in the validating functions. Should simplify the initializer method a bit.

I see also some design issues:

  • Having two property names.
    The typed_property decorator needs to have the property name twice, as an assignment on left side and as a string in the argument. It's probably not much, but it feels a bit un-pythonic.

  • Need two callables for the typed_property decorator.
    Although this is not much either, it is also feels a bit awkward, especially with the lambda for prerelease and build.

  • Validating functions outside the class
    Currently, the validating functions are outside of the Version class. I see currently no solution to integrate that into the class as that a forward reference would be needed. Theoretically, we could do that in __init__ (or even __new__) but that would complicate the matter.

Going Beyond (Descriptor Protocol)

The above issues could be solved. Python has for this kind of problems the descriptor protocol.

Properties are just an implementation of the more general descriptor protocol. That is a class which contains one of the methods __get__, __set__ or __delete__, see https://docs.python.org/3/howto/descriptor.html

Theoretically, we could add an additional class which implements the above behavior in the said methods. This would be a charming way as all the checking/validating is hidden inside the class.

However, I'm not sure to introduce another class just to implement that. It seems also a bit of an overkill to me, although it's quite pythonic. Not sure if we would introduce other problems as it raises the complexity a bit.
On the other side, it could be beneficial if we (or another user) want to derive the Version class with some other validating parts.

Related Information


@python-semver/reviewers @tlaferriere Any comments? Thoughts?

Metadata

Metadata

Assignees

No one assigned

    Labels

    DesignIdeas, suggestions, musings about design questionsIdeas, suggestions, musings about design questionsEnhancementNot a bug, but increases or improves in value, quality, desirability, or attractivenessNot a bug, but increases or improves in value, quality, desirability, or attractivenessQuestionUnclear or open issue subject for debateUnclear or open issue subject for debateRelease_3.x.yOnly for the major release 3Only for the major release 3

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions

      Morty Proxy This is a proxified and sanitized view of the page, visit original site.