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

Fix incorrect event index for slice with stop < start #999

Merged
merged 1 commit into from
Apr 9, 2020

Conversation

mdickinson
Copy link
Member

Fixes #994.

Checklist

  • Tests

@mdickinson mdickinson requested a review from kitchoi April 9, 2020 07:46
@codecov-io
Copy link

codecov-io commented Apr 9, 2020

Codecov Report

Merging #999 into master will increase coverage by 1.04%.
The diff coverage is 96.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #999      +/-   ##
==========================================
+ Coverage   73.05%   74.10%   +1.04%     
==========================================
  Files          51       51              
  Lines        6514     6453      -61     
  Branches     1309     1277      -32     
==========================================
+ Hits         4759     4782      +23     
+ Misses       1363     1305      -58     
+ Partials      392      366      -26     
Impacted Files Coverage Δ
traits/trait_base.py 62.98% <ø> (-0.60%) ⬇️
traits/trait_types.py 71.72% <88.88%> (+0.07%) ⬆️
traits/trait_list_object.py 97.48% <97.68%> (+23.20%) ⬆️
traits/ctrait.py 61.98% <100.00%> (+3.42%) ⬆️
traits/has_traits.py 71.31% <100.00%> (+0.07%) ⬆️
traits/trait_handlers.py 61.11% <100.00%> (ø)
traits/trait_type.py 77.21% <100.00%> (-1.10%) ⬇️
traits/traits.py 76.63% <100.00%> (+3.76%) ⬆️
traits/editor_factories.py 79.59% <0.00%> (-9.19%) ⬇️
... and 2 more

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 b0c013d...36f4606. Read the comment docs.

@kitchoi
Copy link
Contributor

kitchoi commented Apr 9, 2020

Thanks! It is interesting to see how this bug-for-bug implementation changes face over time. For reference, the history started here: cc83835
then this: 010b774

Now the if key.start is None is moved to _normalize_slice, that line is back to how it was again.

Copy link
Contributor

@kitchoi kitchoi left a comment

Choose a reason for hiding this comment

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

LGTM

@mdickinson
Copy link
Member Author

this bug-for-bug implementation changes face over time

Agreed. I don't think we actually have bugs in this part of TraitListObject any more, and there's no need for bug-for-bug compatibility code at this point. I actually want to change TraitList to match TraitListObject here. (See comments in #997)

@mdickinson mdickinson merged commit 3075865 into master Apr 9, 2020
@mdickinson mdickinson deleted the fix/setitem-bad-indx branch April 9, 2020 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TraitListObject gives bad index for __setitem__
3 participants