Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Make TraitList notify index a plain int where possible #1003

Merged
merged 3 commits into from
Apr 16, 2020
Merged

Make TraitList notify index a plain int where possible #1003

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
23f284e
Prefer plain integer indices where that makes sense
mdickinson Apr 15, 2020
e765636
Extend changes to __delitem__ and __setitem__
mdickinson Apr 15, 2020
024270a
Remove 'bug-for-bug conversion' comment; we no longer need to do anyt…
mdickinson Apr 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 49 additions & 17 deletions traits/tests/test_trait_list_object.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ def test_setitem(self):
self.assertEqual(self.added, [5])

tl[:] = [1, 2, 3, 4, 5]
self.assertEqual(self.index, slice(0, 3, None))
self.assertEqual(self.index, 0)
self.assertEqual(self.removed, [1, 5, 3])
self.assertEqual(self.added, [1, 2, 3, 4, 5])

Expand Down Expand Up @@ -270,7 +270,7 @@ def test_setitem_iterable(self):

tl[:] = (x**2 for x in range(4))
self.assertEqual(tl, [0, 1, 4, 9])
self.assertEqual(self.index, slice(0, 3))
self.assertEqual(self.index, 0)
self.assertEqual(self.removed, [1, 2, 3])
self.assertEqual(self.added, [0, 1, 4, 9])

Expand Down Expand Up @@ -375,7 +375,7 @@ def test_setitem_corner_case(self):
# Note: new items inserted at position 5, not position 2.
tl[5:2] = [10, 11, 12]
self.assertEqual(tl, [0, 1, 2, 3, 4, 10, 11, 12, 5, 6])
self.assertEqual(self.index, slice(5, 2))
self.assertEqual(self.index, 5)
self.assertEqual(self.removed, [])
self.assertEqual(self.added, [10, 11, 12])

Expand All @@ -390,7 +390,7 @@ def test_delitem(self):
self.assertEqual(self.added, [])

del tl[:]
self.assertEqual(self.index, slice(0, 2, None))
self.assertEqual(self.index, 0)
self.assertEqual(self.removed, [1, 2])
self.assertEqual(self.added, [])

Expand Down Expand Up @@ -425,7 +425,7 @@ def test_iadd(self):
notifiers=[self.notification_handler])

tl += [6, 7]
self.assertEqual(self.index, slice(2, 4, None))
self.assertEqual(self.index, 2)
self.assertEqual(self.removed, [])
self.assertEqual(self.added, [6, 7])

Expand All @@ -448,7 +448,7 @@ def test_iadd_converts(self):

tl += [True, True]
self.assertEqual(tl, [4, 5, 1, 1])
self.assertEqual(self.index, slice(2, 4))
self.assertEqual(self.index, 2)
self.assertEqual(self.removed, [])
self.assertEqual(self.added, [1, 1])
self.assertTrue(
Expand Down Expand Up @@ -478,7 +478,7 @@ def test_iadd_iterable(self):

tl += (x**2 for x in range(3))
self.assertEqual(tl, [4, 5, 0, 1, 4])
self.assertEqual(self.index, slice(2, 5))
self.assertEqual(self.index, 2)
self.assertEqual(self.removed, [])
self.assertEqual(self.added, [0, 1, 4])

Expand All @@ -494,7 +494,7 @@ def test_imul(self):
self.assertEqual(self.added, None)

tl *= 2
self.assertEqual(self.index, slice(2, 4, None))
self.assertEqual(self.index, 2)
self.assertEqual(self.removed, [])
self.assertEqual(self.added, [1, 2])

Expand All @@ -505,7 +505,7 @@ def test_imul(self):
tl *= 2.5

tl *= -1
self.assertEqual(self.index, slice(0, 4, None))
self.assertEqual(self.index, 0)
self.assertEqual(self.removed, [1, 2, 1, 2])
self.assertEqual(self.added, [])

Expand All @@ -531,13 +531,13 @@ def test_imul_integer_like(self):

tl *= numpy.int64(2)
self.assertEqual(tl, [1, 2, 1, 2])
self.assertEqual(self.index, slice(2, 4))
self.assertEqual(self.index, 2)
self.assertEqual(self.removed, [])
self.assertEqual(self.added, [1, 2])

tl *= numpy.int64(-1)
self.assertEqual(tl, [])
self.assertEqual(self.index, slice(0, 4))
self.assertEqual(self.index, 0)
self.assertEqual(self.removed, [1, 2, 1, 2])
self.assertEqual(self.added, [])

Expand Down Expand Up @@ -597,7 +597,7 @@ def test_extend(self):
notifiers=[self.notification_handler])

tl.extend([1, 2])
self.assertEqual(self.index, slice(1, 3, None))
self.assertEqual(self.index, 1)
self.assertEqual(self.removed, [])
self.assertEqual(self.added, [1, 2])

Expand All @@ -620,7 +620,7 @@ def test_extend_converts(self):

tl.extend([False, True])
self.assertEqual(tl, [4, 0, 1])
self.assertEqual(self.index, slice(1, 3))
self.assertEqual(self.index, 1)
self.assertEqual(self.removed, [])
self.assertEqual(self.added, [0, 1])
self.assertTrue(
Expand Down Expand Up @@ -650,7 +650,7 @@ def test_extend_iterable(self):

tl.extend(x**2 for x in range(10, 13))
self.assertEqual(tl, [1, 100, 121, 144])
self.assertEqual(self.index, slice(1, 4))
self.assertEqual(self.index, 1)
self.assertEqual(self.removed, [])
self.assertEqual(self.added, [100, 121, 144])

Expand Down Expand Up @@ -755,7 +755,7 @@ def test_clear(self):
item_validator=int_item_validator,
notifiers=[self.notification_handler])
tl.clear()
self.assertEqual(self.index, slice(0, 5, None))
self.assertEqual(self.index, 0)
self.assertEqual(self.removed, [1, 2, 3, 4, 5])
self.assertEqual(self.added, [])

Expand All @@ -778,21 +778,53 @@ def test_sort(self):
tl.sort()

self.assertEqual(tl, [0, 1, 2, 3, 4, 5])
self.assertEqual(self.index, slice(0, 6, None))
self.assertEqual(self.index, 0)
self.assertEqual(self.removed, [2, 3, 1, 4, 5, 0])
self.assertEqual(self.added, [0, 1, 2, 3, 4, 5])

def test_sort_empty_list(self):
tl = TraitList([],
item_validator=int_item_validator,
notifiers=[self.notification_handler])

tl.sort()
self.assertIsNone(self.index)
self.assertIsNone(self.removed)
self.assertIsNone(self.added)

def test_sort_already_sorted(self):
tl = TraitList([10, 11, 12, 13, 14],
item_validator=int_item_validator,
notifiers=[self.notification_handler])

tl.sort()

self.assertEqual(tl, [10, 11, 12, 13, 14])
self.assertEqual(self.index, 0)
self.assertEqual(self.removed, [10, 11, 12, 13, 14])
self.assertEqual(self.added, [10, 11, 12, 13, 14])

def test_reverse(self):
tl = TraitList([1, 2, 3, 4, 5],
item_validator=int_item_validator,
notifiers=[self.notification_handler])

tl.reverse()
self.assertEqual(tl, [5, 4, 3, 2, 1])
self.assertEqual(self.index, slice(0, 5, None))
self.assertEqual(self.index, 0)
self.assertEqual(self.removed, [1, 2, 3, 4, 5])
self.assertEqual(self.added, [5, 4, 3, 2, 1])

def test_reverse_empty_list(self):
tl = TraitList([],
item_validator=int_item_validator,
notifiers=[self.notification_handler])

tl.reverse()
self.assertIsNone(self.index)
self.assertIsNone(self.removed)
self.assertIsNone(self.added)

def test_reverse_single_notification(self):
# Regression test for double notification.
notifier = unittest.mock.Mock()
Expand Down
36 changes: 20 additions & 16 deletions traits/trait_list_object.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,18 @@ def _normalize_index(index, length):
def _normalize_slice(index, length):
""" Normalize slice start and stop to range 0 to len (inclusive). """

# Do not normalize if step is negative.
# Non-extended slice (step = 1): return only need the start index.
if index.step is None or index.step == 1:
if index.start is None:
return 0
else:
return _normalize_index(index.start, length)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still attempting to support whatever was intended when the bug-for-bug implementation was put in place?: cc83835
Is that "bug" still relevant or are we blessing this as a feature?

(I just realized that previously in traits 5.2.0, the bug-for-bug implementation would not have normalized a negative index.start, though I doubt that was the intended bug-for-bug code.)

Copy link
Member Author

Choose a reason for hiding this comment

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

So I think there aren't actually any bugs in the TraitListObject that we need to reproduce at this point, so need for bug-for-bug compatibility. IOW, yes, we're blessing this as a feature.

Copy link
Member Author

Choose a reason for hiding this comment

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

And yes, there are going to be some small behaviour changes here. So long as we do the usual level of careful testing with respect to Traits-using projects before we release, I'm happy that those changes don't represent a serious risk of breakage.

Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM!


# Extended slice with negative step: do not normalize.
if index.step is not None and index.step < 0:
return index

# Extended slice with positive step: normalize the start and stop.
return slice(
_normalize_index(0 if index.start is None else index.start, length),
_normalize_index(length if index.stop is None else index.stop, length),
Expand Down Expand Up @@ -200,7 +208,7 @@ def __iadd__(self, value):
added = [self.item_validator(item) for item in value]
extended = super().__iadd__(added)
if added:
self.notify(slice(original_length, len(self)), [], added)
self.notify(original_length, [], added)
return extended

def __imul__(self, value):
Expand All @@ -221,14 +229,13 @@ def __imul__(self, value):
removed = self.copy()
multiplied = super().__imul__(value)
if removed:
self.notify(slice(0, len(removed)), removed, [])
self.notify(0, removed, [])
else:
original_length = len(self)
multiplied = super().__imul__(value)
new_length = len(self)
if new_length > original_length:
index = slice(original_length, new_length)
self.notify(index, [], self[index])
added = self[original_length:]
if added:
self.notify(original_length, [], added)
return multiplied

def __setitem__(self, key, value):
Expand Down Expand Up @@ -290,7 +297,7 @@ def clear(self):
removed = self.copy()
super().clear()
if removed:
self.notify(slice(0, len(removed)), removed, [])
self.notify(0, removed, [])

def extend(self, iterable):
""" Extend list by appending elements from the iterable.
Expand All @@ -305,7 +312,7 @@ def extend(self, iterable):
added = [self.item_validator(item) for item in iterable]
super().extend(added)
if added:
self.notify(slice(original_length, len(self)), [], added)
self.notify(original_length, [], added)

def insert(self, index, object):
""" Insert object before index.
Expand Down Expand Up @@ -382,7 +389,8 @@ def reverse(self):
""" Reverse the items in the list in place. """
removed = self.copy()
super().reverse()
self.notify(slice(0, len(self)), removed, self.copy())
if removed:
self.notify(0, removed, self.copy())

def sort(self, *, key=None, reverse=False):
""" Sort the list in ascending order and return None.
Expand All @@ -405,7 +413,8 @@ def sort(self, *, key=None, reverse=False):
"""
removed = self.copy()
super().sort(key=key, reverse=reverse)
self.notify(slice(0, len(self)), removed, self.copy())
if removed:
self.notify(0, removed, self.copy())

# -- pickle and copy support ----------------------------------------------

Expand Down Expand Up @@ -509,11 +518,6 @@ def notifier(self, trait_list, index, removed, added):
if object is None:
return

# bug-for-bug conversion of parameters to TraitListEvent
if isinstance(index, slice):
if index.step is None or index.step == 1:
index = index.start

event = TraitListEvent(index, removed, added)
items_event = self.trait.items_event()
object.trait_items_event(self.name_items, event, items_event)
Expand Down