-
Notifications
You must be signed in to change notification settings - Fork 96
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
Conversation
# Conflicts: # semver.py
…ility function for check the validity of the index.
…w useless _validate_index.
Co-Authored-By: Tom Schraitle <tomschr@users.noreply.github.com>
� Conflicts: � test_semver.py
# Conflicts: # semver.py # test_semver.py
… being thrown every time it should.
# Conflicts: # CHANGELOG.rst
Yet again I have not cleaned up my fork, so please squash commits as you merge 😉 |
None
on falsy partsNone
on falsy parts
There was a problem hiding this 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! 👍
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>
I added some cases to
and I noticed that it gave us the wrong error ( |
@tlaferriere This is great, thanks for your efforts 👍 I forget to tell you that we also need to update the 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 |
Co-authored-by: Tom Schraitle <tomschr@users.noreply.github.com>
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. |
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.