Add `getitem` method to allow easy access to version parts #138

tlaferriere · Aug 16, 2019

This is a natural extension of the work done in #124 (and parents) and makes it easy to access version parts using their index.

Use case:

Using an Enum to access the version part:

VersionPartEnum = Enum('VersionPartEnum', "MAJOR MINOR PATCH PRERELEASE BUILD")
version_info = VersionInfo.parse("1.2.3-rc.1+build.13")

print(version_info[VersionPartEnum.MINOR - 1])  # 2

s-celles · Aug 17, 2019

Thanks @tlaferriere for this PR
but a feature should never be implemented without unit tests.

tlaferriere · Aug 17, 2019

I'll get on it.

tlaferriere · Aug 19, 2019

@scls19fr Should I add test vectors for edge cases or are these ones satisfactory?

s-celles · Aug 20, 2019

I would be pleased to have the general consensus about this feature before merging
Pinging @k-bx @kxepal @bittner @tomschr @ppkt

bittner · Aug 20, 2019

The implementation is certainly fine.

I'm not super-convinced by the above use case, though. While it's a fact that Enums start at 1 for a reason I'd prefer to see a more intuitive syntax (i.e. something that doesn't need to do index calculations, namely - 1). Then, if this is a real use case, why do we require the user to come up with the needed Enum?

The doctests of parse_version_info may also include access with constants, and we may provide the constants directly, e.g.

>>> import semver
>>> version_info = semver.parse_version_info("3.4.5-pre.2+build.4")
>>> version_info.major == version_info[semver.MAJOR] == 3
True
>>> version_info.minor == version_info[semver.MINOR] == 4
True
...

MAJOR & friends would have to be (read-only) class members for this to work, obviously.

ppkt

@tlaferriere - overall I'm happy with these changes - we are already supporting iteration over VersionInfo instance so, in my opinion, it's a quite nice feature to have an ability to have a support for indexing.

I have only some small remarks regarding your PR.

semver.py

test_semver.py

tomschr · Aug 22, 2019

@bittner I like your idea! 👍 😃

s-celles · Oct 1, 2019

This PR have a conflicting file (sorry I should have managed this one before but was and still is quite busy).
Please resolve conflicts and thumb up if you are ok to merge this.
Thumb down if you don't want this PR to be merged.

# Conflicts: # semver.py

s-celles · Oct 8, 2019

Please fix this conflict to have a chance to have this feature include in latest release!

tlaferriere · Oct 8, 2019

Excuse the delay, I am resolving the conflicts and finishing up tests.

…ility function for check the validity of the index.

tlaferriere · Oct 8, 2019

So I've decided to disallow negative indexing as it would require going against the nature of a tuple to make behave like List in essence. I've tried to cover all edge cases that could come up with a slice in the _validate_index protected method. If you spot edge cases that make it fail let me know 😉.
Edit: just found some edge cases.
Edit again: Changing my mind about negative indexing. Trying to block negative indexing while allowing slices is not easy at all. Too many edge cases that have to be handled manually, which is not very pythonic IMO.

tlaferriere · Oct 8, 2019

Also I've decided not to add the MAJOR, MINOR, etc. members as it feels redundant with the key indices like "major", "minor", etc. After all, the goal was to access it like an iterable, and not a dictionary.

…w useless _validate_index.

s-celles · Oct 8, 2019

Thanks @tlaferriere for this first contribution here.

Could you update doc to show this new feature?
I wonder if we shouldn't have example in docstring also.

Could you also update CHANGELOG?

s-celles · Oct 8, 2019

You can also add your name to https://github.com/k-bx/python-semver/blob/master/CONTRIBUTORS

tomschr · Oct 8, 2019

semver.py

@@ -168,6 +168,12 @@ def __iter__(self):
        for v in self._astuple():
            yield v

+    def __getitem__(self, index):
+        """Implement getitem. This automatically strips empty parts of the
+           version from the iterable from which the index is taken."""


Thanks @tlaferriere, could you change the docstring:

Suggested change

version from the iterable from which the index is taken."""

version from the iterable from which the index is taken.

:param int index: a positive or negative integer indicating

the offset

:raises: IndexError, if index is beyond the range

"""

@tomschr according to current implementation, index can be negative.

@tlaferriere I think there is a subtle bug in your implementation:

def test_example(): v = VersionInfo.parse('1.2.3+build0') > assert v[:] == (1, 2, 3, None, 'build0') E AssertionError: assert (1, 2, 3, 'build0') == (1, 2, 3, None, 'build0') E At index 3 diff: 'build0' != None E Right contains more items, first extra item: 'build0' E Use -v to get the full diff

i.e. if "prerelease" part is missing and "metadata" is present, after using getitem , "metadata" appears at index for "prerelease"

@ppkt

according to current implementation, index can be negative.

You are right, I've corrected the GH suggestion. Thanks! 👍

After much deliberation with myself and an increasingly complex function, I have decided to rewrite it (multiple times). I believe I have made this implementation as simple as possible. Turns out the subtle bug mentioned by @ppkt was a symptom of a few bad design decisions on my part. Now it doesn't support negative indices.

Co-Authored-By: Tom Schraitle <tomschr@users.noreply.github.com>

s-celles · Oct 13, 2019

Conflicting files need to be solved here also. CI is failing and should be fixed also.

� Conflicts: � test_semver.py

…tual features.

bittner · Nov 20, 2019

semver.py

+           :param Union[int, slice] index: a positive integer indicating the
+                  offset or a slice
+           :raises: IndexError, if index is beyond the range or a part is None
+           """


This docstring has an unusual indentation. According to PEP 257 indentation may look like:

def foo(): """First line of comment Second line of comment. The longer description. """

(what I like less but seems to be favored by Guido & friends), or:

def foo(): """ First line of comment Second line of comment. The longer description. """

Same below, in the method following next.

Good point, but then the other docstrings needs to be adapted as well. Maybe we could do that in a combined issue later?

bittner · Nov 20, 2019

semver.py

+            else:
+                slice_is_full = True
+                part = self._astuple()[index]
+                for i in part:


What is i here? Pylint would be complaining (if you ran it). Can you use a self-explanatory name?

One idea could be to replace i with item.

I guess I could use subpart instead.

See also the more recent comments below. 😉

bittner · Nov 20, 2019

semver.py

+                raise negative_error
+            else:
+                part = self._astuple()[index]
+                if part is None:


if not part:

... is probably more pythonic. Or is there a reason for an explicit None?

This is in several places in the code, by the way, and could be made pythonic throughout the code.

tomschr

Just a small comment. 😉

tomschr · Nov 20, 2019

semver.py

+           :param Union[int, slice] index: a positive integer indicating the
+                  offset or a slice
+           :raises: IndexError, if index is beyond the range or a part is None
+           """


Good point, but then the other docstrings needs to be adapted as well. Maybe we could do that in a combined issue later?

tomschr · Nov 20, 2019

@tlaferriere Found another issue.

You could simplify the code a bit and omit the else branches. Could help to improve readability:

def __getitem__(self, index):
        """
           Implement getitem. If the part requested is undefined, or a part of the
           range requested is undefined, it will throw an index error.
           Negative indices are not supported

           :param Union[int, slice] index: a positive integer indicating the
                  offset or a slice
           :raises: IndexError, if index is beyond the range or a part is None
           """
        undefined_error = IndexError("Version part undefined")
        negative_error = IndexError("Version index cannot be negative")
        if isinstance(index, slice):
            if (False if index.start is None else index.start < 0) or \
                    (False if index.stop is None else index.stop < 0):
                raise negative_error
            slice_is_full = True
            part = self._astuple()[index]
            for item in part:
                if item is None:
                    slice_is_full = False
                elif not slice_is_full:
                    raise undefined_error
            part = tuple(filter(None, part))
        else:
            if index < 0:
                raise negative_error
            part = self._astuple()[index]
            if part is None:
                raise undefined_error
        return part

tomschr

See #138 (comment)

tomschr · Nov 24, 2019

@tlaferriere
I've played around with the code a bit and find a quite readable version. It reduces the line count from 21 to 12. 👀 It also avoids the for-loop. Maybe you can try it out?

def __getitem__(self, index):
        """Implement getitem. If the part requested is undefined, or a part of the
           range requested is undefined, it will throw an index error.
           Negative indices are not supported

           :param Union[int, slice] index: a positive integer indicating the
                  offset or a slice
           :raises: IndexError, if index is beyond the range or a part is None
           :return: the requested part of the version at position index
        """
        if isinstance(index, int):
            index = slice(index, index+1)

        if isinstance(index, slice) and \
           (index.start is None or index.start < 0) and \
           (index.stop is None or index.stop < 0):
            raise IndexError("Version index cannot be negative")

        # Could raise IndexError:
        part = tuple(filter(None, self._astuple()[index]))

        if len(part) == 1:
            part = part[0]
        if not part:
            raise IndexError("Version part undefined")
        return part

Tests were successful. 🎉

tomschr · Dec 10, 2019

@tlaferriere Thomas, what's your opinion on #138 (comment)?

Could we finish this PR to publish it for the 2.9.1 release or do you need more time? 😉 If you don't have time yet due to the holiday season, I could finish it for you if you like. Let me know what you prefer. 😉

tlaferriere · Dec 10, 2019

@tomschr with my finals ending soon, I figured I would have more time to finish it up and get it ready for the release. I like the snippet, I haven't had time to inspect it, but it sure seems cleaner. I'll get some work in the next few days and hopefully it will go well enough for it to be ready.

tomschr · Dec 11, 2019

Take your time. If you need any help, let us know. 😊

tomschr · Feb 20, 2020

@tlaferriere Thomas, just a quick question: if you are too busy at the moment (which is completely fine!), should I finish the issue for you or would you prefer to do it yourself? Of course I will add you as co-author. 😉

* Add __getitem__ to VersionInfo class * Add test cases * Add user documentation * Extend CHANGELOG Co-authored-by: Thomas Laferriere <tlaferriere@users.noreply.github.com> Co-authored-by: Peter Bittner <bittner@users.noreply.github.com> Co-authored-by: Karol Werner <ppkt@users.noreply.github.com> Co-authored-by: Sébastien Celles <scls19fr@users.noreply.github.com>

tomschr · Apr 26, 2020

I've created this PR anew in #243, so closing this.

Implement __getitem__ for #138

Add __getitem__ method to allow easy access to version parts

f1d7cde

tlaferriere added 2 commits August 19, 2019 09:56

Add unit tests for index access to version parts

ba5899e

Remove whitespace on blank line

e88e0ba

ppkt approved these changes Aug 20, 2019

View reviewed changes

semver.py Outdated Show resolved Hide resolved

test_semver.py Show resolved Hide resolved

s-celles mentioned this pull request Oct 1, 2019

Publish a new release on PyPi (2.9.0) #134

Closed

Merge remote-tracking branch 'kbx/master'

c30d38f

# Conflicts: # semver.py

Merge upstream into fork and fix conflicts

26a7e03

Fix stash conflicts and create more test vectors. Also add private ut…

80da20c

…ility function for check the validity of the index.

Use simpler implementation that allows negative indices and remove no…

a610c22

…w useless _validate_index.

tomschr reviewed Oct 8, 2019

View reviewed changes

Add param and raises to __getitem__ docstring

95e79d8

Co-Authored-By: Tom Schraitle <tomschr@users.noreply.github.com>

tlaferriere added 5 commits November 2, 2019 16:47

Merge branch 'master' of github.com:k-bx/python-semver

ab35e38

� Conflicts: � test_semver.py

Reiterate the logic for __getitem__ and update tests to follow the ac…

27632b7

…tual features.

Update __getitem__ docstring

976a4ee

Fix lint issues

cf65b3d

Fix lint issues

dabebd9

s-celles requested a review from tomschr November 20, 2019 05:29

bittner suggested changes Nov 20, 2019

View reviewed changes

tomschr reviewed Nov 20, 2019

View reviewed changes

s-celles requested a review from tomschr November 20, 2019 20:34

tomschr reviewed Nov 20, 2019

View reviewed changes

tomschr mentioned this pull request Nov 24, 2019

Fix #201: Reformat source code with black #202

Merged

tomschr mentioned this pull request Apr 26, 2020

Implement __getitem__ for #138 #243

Merged

tomschr closed this Apr 26, 2020

tomschr added a commit that referenced this pull request May 1, 2020

Merge pull request #243 from tomschr/feature/138-add-__getitem__

2ef2eea

Implement __getitem__ for #138

tomschr mentioned this pull request May 1, 2020

Create new 2.10.0 Release #237

Closed

5 tasks

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

Add __getitem__ method to allow easy access to version parts #138

Add __getitem__ method to allow easy access to version parts #138

Uh oh!

Conversation

tlaferriere commented Aug 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Use case:

Uh oh!

s-celles commented Aug 17, 2019

Uh oh!

tlaferriere commented Aug 17, 2019

Uh oh!

tlaferriere commented Aug 19, 2019

Uh oh!

s-celles commented Aug 20, 2019

Uh oh!

bittner commented Aug 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ppkt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tomschr commented Aug 22, 2019

Uh oh!

s-celles commented Oct 1, 2019

Uh oh!

s-celles commented Oct 8, 2019

Uh oh!

tlaferriere commented Oct 8, 2019

Uh oh!

tlaferriere commented Oct 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tlaferriere commented Oct 8, 2019

Uh oh!

s-celles commented Oct 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

s-celles commented Oct 8, 2019

Uh oh!

tomschr Oct 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s-celles commented Oct 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomschr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomschr commented Nov 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomschr left a comment

Add `getitem` method to allow easy access to version parts #138

Add `getitem` method to allow easy access to version parts #138

tlaferriere commented Aug 16, 2019 •

edited

Loading

bittner commented Aug 20, 2019 •

edited

Loading

tlaferriere commented Oct 8, 2019 •

edited

Loading

s-celles commented Oct 8, 2019 •

edited

Loading

tomschr Oct 8, 2019 •

edited

Loading

tomschr commented Nov 20, 2019 •

edited

Loading

tomschr commented Nov 24, 2019 •

edited

Loading

tomschr commented Feb 20, 2020 •

edited

Loading