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.

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

TraitListObject misformats items_changed events on delete with extended slice #742

Closed
k2bd opened this issue Jan 15, 2020 · 1 comment · Fixed by #771
Closed

TraitListObject misformats items_changed events on delete with extended slice #742

k2bd opened this issue Jan 15, 2020 · 1 comment · Fixed by #771
Assignees
Milestone

Comments

@k2bd
Copy link
Contributor

k2bd commented Jan 15, 2020

TraitListObject.__delitem__ handles slices with steps !=1 specially:

else:
index = key
removed = [removed]

This produces e.g. [[9, 5]] instead of [9, 5] for the removed part of the event on these lines:

del self.obj.alist[1:4:2]
self.assertLastTraitListEventEqual(slice(1, 4, 2), [[9, 5]], [])

This doesn't match the behaviour found when e.g. list items are changed:

self.obj.alist[0:2:1] = [8, 9]
self.assertLastTraitListEventEqual(0, [4, 5], [8, 9])

Discussed offline, the current behaviour is intentional to reproduce legacy results

@mdickinson mdickinson added this to the 6.0.0 release milestone Jan 15, 2020
@mdickinson
Copy link
Member

If we're going to change this, 6.0 might be a good release to do it in. It's technically a backwards-compatibility break, but it's one that's unlikely to affect much real code (deleting slices with a step != 1 is rare).

Note that this is not just about __delitem__, but about slice assignment, too:

>>> from traits.api import *
>>> class A(HasTraits):
...     foo = List(Int)
...     def _foo_items_changed(self, list_event):
...         print(f"Event: index={list_event.index}, added={list_event.added}, removed={list_event.removed}")
... 
>>> a = A()
>>> a.foo 
[]
>>> a.foo.extend([1, 2, 3])
Event: index=0, added=[1, 2, 3], removed=[]
>>> a.foo[::2] = [4, 5]
Event: index=slice(None, None, 2), added=[[4, 5]], removed=[[1, 3]]
>>> del a.foo[::]
Event: index=0, added=[], removed=[4, 2, 5]

I'm +1 on changing this for 6.0.

There are some other oddities/inconsistencies, too: note the difference between added and removed in the following:

>>> del a.foo[::-1]
Event: index=slice(None, None, -1), added=[], removed=[[]]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Enthought OSS
  
Done
Development

Successfully merging a pull request may close this issue.

2 participants