Skip to content

Fix #260 __getitem__ returning None on falsy parts #261

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

Merged
merged 26 commits into from
Jun 10, 2020
Merged

Fix #260 __getitem__ returning None on falsy parts #261

merged 26 commits into from
Jun 10, 2020

Conversation

tlaferriere
Copy link
Contributor

For reference here is the behavior that caused the problem in the first.

I also found that the IndexError wasn't being raised every time it should because it was reliant on the faulty behavior descibed above for some edge cases.

tlaferriere and others added 21 commits August 16, 2019 16:39
…ility function for check the validity of the index.
Co-Authored-By: Tom Schraitle <tomschr@users.noreply.github.com>
# Conflicts:
#	semver.py
#	test_semver.py
# Conflicts:
#	CHANGELOG.rst
@tlaferriere
Copy link
Contributor Author

Yet again I have not cleaned up my fork, so please squash commits as you merge 😉

@tlaferriere tlaferriere changed the title Fix __getitem__ returning None on falsy parts Fix #260 __getitem__ returning None on falsy parts Jun 9, 2020
Copy link
Member

@tomschr tomschr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @tlaferriere, thanks for your contribution! 👍 I have some minor comments, could you check them if they are okay? Other than that, it's fine. Thanks! 👍

@tomschr tomschr linked an issue Jun 9, 2020 that may be closed by this pull request
@tomschr
Copy link
Member

tomschr commented Jun 9, 2020

I've played around a bit and it seems, I could check for negative indices in the if clause around line 549:

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

this makes your first if clause obsolete.

Furthermore, I think the test cases doesn't distinguish enough between the error messages. Maybe we should rework them, I would suggest these additional changes:

@pytest.mark.parametrize(
    "version, index",
    [
        ("1.2.3", 3),
        ("1.2.3", slice(3, 4)),
        ("1.2.3", 4),
        ("1.2.3", slice(4, 5)),
        ("1.2.3", 5),
        ("1.2.3", slice(5, 6)),
        ("1.2.3-rc.0", 5),
        ("1.2.3-rc.0", slice(5, 6)),
        ("1.2.3-rc.0", 6),
        ("1.2.3-rc.0", slice(6, 7)),
    ],
)
def test_version_info_should_throw_index_error(version, index):
    version_info = VersionInfo.parse(version)
    with pytest.raises(IndexError, match=r"Version part undefined"):
        version_info[index]

@pytest.mark.parametrize(
    "version, index",
    [
        ("1.2.3", -1),
        ("1.2.3", -2),
    ],
)
def test_version_info_should_throw_index_error_when_negative_index(version, index):
    version_info = VersionInfo.parse(version)
    with pytest.raises(IndexError, match=r"Version index cannot be negative"):
        version_info[index]

Co-authored-by: Tom Schraitle <tomschr@users.noreply.github.com>
@tlaferriere
Copy link
Contributor Author

I added some cases to test_version_info_should_throw_index_error_when_negative_index:

        ("1.2.3", slice(-2, 2)),
        ("1.2.3", slice(2, -2)),
        ("1.2.3", slice(-2, -2)),

and I noticed that it gave us the wrong error ("Version part undefined"), so I changed the second if logic and now it works perfectly.

@tomschr
Copy link
Member

tomschr commented Jun 9, 2020

@tlaferriere This is great, thanks for your efforts 👍 I forget to tell you that we also need to update the CHANGELOG.rst file. Could you amend it?
To make it easier for you, here is the text snippet to copy and paste (add it before the Version 2.10.1 line):

Version 2.10.2 (WIP)
====================

:Released: 2020-xx-yy
:Maintainer:

Features
--------

n/a

Bug Fixes
---------

:gh:`260` (:pr:`261`): Fixed ``__getitem__`` returning None on wrong parts


Additions
---------

n/a

Removals
--------

n/a

Thomas Laferriere and others added 2 commits June 9, 2020 17:51
Co-authored-by: Tom Schraitle <tomschr@users.noreply.github.com>
@tomschr tomschr merged commit f332326 into python-semver:master Jun 10, 2020
@tomschr tomschr added the Bug Error, flaw or fault to produce incorrect or unexpected results label Jun 10, 2020
@tomschr
Copy link
Member

tomschr commented Jun 10, 2020

Thank you @tlaferriere for your efforts! ❤️ I've squashed the commits and merged it to master.

May I give you a small hint about keeping commits clean? 😉 It looks like you haven't synced your fork with the latest changes from upstream. Find more information in the GitHub documentation Keep your fork synced.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Error, flaw or fault to produce incorrect or unexpected results
Projects
None yet
Development

Successfully merging this pull request may close these issues.

__getitem__ returns None for version parts that are falsy
2 participants