Further index normalization in TraitList

[mdickinson]

This PR builds on #1003 and:

• Adds further index normalization for __delitem__ and __setitem__ operations on TraitList
• Adds a general test mechanism for checking that a list operation can be reconstructed from the (index, removed, added) triple.
• Adds some exhaustive testing for __delitem__ and __setitem__ operations. These tests are slow, so we may want to kill them at some point.

The key part of the tests are these lines, which show how to reconstruct a list operation from the (index, removed, added) triple:

traits/traits/tests/test_trait_list_object.py

Lines 1016 to 1029 in 9a74866

	# Check that we can reconstruct the list operation from the event.
	reconstructed = original_list.copy()
	if isinstance(index, slice):
	self.assertEqual(removed, reconstructed[index])
	if added:
	reconstructed[index] = added
	else:
	del reconstructed[index]
	else:
	removed_slice = slice(index, index + len(removed))
	self.assertEqual(removed, reconstructed[removed_slice])
	del reconstructed[removed_slice]
	reconstructed[index:index] = added
	self.assertEqual(reconstructed, trait_list)

[mdickinson]

Here's an example showing the main effect:

>>> from traits.api import HasTraits, List
>>> class A(HasTraits):
...     foo = List()
...     def _foo_items_changed(self, event):
...         print(event.__dict__)
... 
>>> a = A(foo=list(range(10)))
>>> a
<__main__.A object at 0x10d9d99f0>
>>> a.foo
[0, 1, 2, 3, 4, 5, 6, 7, 8, 9]
>>> a.foo[7::-2] = [10, 11, 12, 13]  # note reversal of the 'index' below
{'index': slice(1, 8, 2), 'removed': [1, 3, 5, 7], 'added': [13, 12, 11, 10]}

[codecov-io]

Codecov Report

Merging #1009 into master will increase coverage by 0.18%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1009      +/-   ##
==========================================
+ Coverage   73.95%   74.13%   +0.18%     
==========================================
  Files          51       51              
  Lines        6453     6460       +7     
  Branches     1277     1281       +4     
==========================================
+ Hits         4772     4789      +17     
+ Misses       1318     1305      -13     
- Partials      363      366       +3     

Impacted Files	Coverage Δ
traits/trait_list_object.py	97.56% <100.00%> (+0.07%)	⬆️
traits/editor_factories.py	79.59% <0.00%> (-9.19%)	⬇️
traits/trait_type.py	77.21% <0.00%> (-0.64%)	⬇️
traits/trait_types.py	71.72% <0.00%> (-0.15%)	⬇️
traits/trait_handlers.py	61.11% <0.00%> (+0.42%)	⬆️
traits/traits.py	76.63% <0.00%> (+4.09%)	⬆️
traits/etsconfig/etsconfig.py	63.46% <0.00%> (+6.41%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3075865...8098431. Read the comment docs.

[kitchoi]

LGTM, I am happy to see the exhaustive test. While it takes some time to run, it really isn't that long. It would be useful to keep them for it is so easy to get slice indices wrong.

Since the _normalize_slice_or_index is a standalone function, it is possible to write a small unit test for it. I even tried setting the list length to be shorter than the maximum index. It is beyond our use case to have the length being too small, but the test passes:

    def test_slices_equivalence(self):
        length = 6
        test_list = list(range(length))

        # maximum index is larger than the list length, but that's okay
        for test_slice in self.all_slices(max_index=10):
            new_test_list = test_list[test_slice]

            reversed, normalized_index = _normalize_slice_or_index(test_slice, length)
            if isinstance(normalized_index, int):
                normalized_index = slice(normalized_index, normalized_index + len(new_test_list))
            else:
                self.check_index_normalized(normalized_index, length)

            new_list = test_list[normalized_index]
            if reversed:
                new_list = new_list[::-1]
            self.assertEqual(new_test_list, new_list)

[mdickinson]

@kitchoi: Thanks for the review. A couple of followups:

It would be useful to keep them for it is so easy to get slice indices wrong.

Okey-dokey, let's do that.

Since the _normalize_slice_or_index is a standalone function, it is possible to write a small unit test for it.

Yes, I can do that. It makes me slightly twitchy to write unit tests for a private function, but I think we definitely don't want to elevate this function to public at this point: we don't want external users to start using it and relying on its behaviour.

[mdickinson]

Pushed a change from this morning which updated the docstring of _normalize_slice_or_index to be more complete and more accurate. I'll work on the rest of the changes later.

[ievacerny]

Would this PR be a good place to update the FIXME comment in this test? It seems outdated.

[mdickinson]

Yes! I'll do that. It makes documenting the new behaviour all the more important, though.

[mdickinson]

and yes, the "wrapped in another list" bit is gone.

[mdickinson]

Done.

[kitchoi]

LGTM!

[mdickinson]

Resolving conflicts with master ...