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

list/tuple/sequence: implement Index #1825

Merged
merged 3 commits into from Aug 24, 2021
Merged

list/tuple/sequence: implement Index #1825

merged 3 commits into from Aug 24, 2021

Conversation

birkenfeld
Copy link
Member

See #1667

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

👍 looks like a great start!

What do you think of supporting full slice indexing? I think this means either implementing SliceIndex or just implementing Index directly for Range<usize>, RangeFull, RangeInclusive<usize> etc.

@birkenfeld
Copy link
Member Author

Yeah, I though about it, but that runs into API consistency again: do we want to panic on invalid indices or not?
In the end, I didn't (and don't) care about it enough to make the change.

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

That's fair enough. I think for out of bounds slice indexing we should probably panic (to match Rust), given that we already deviated from Python by not supporting negative indexing. That also has the nice dual that again get_slice is the non-panicking companion, even if it doesn't return an error.

I'm happy to implement SliceIndex myself in a follow-up.

@birkenfeld birkenfeld merged commit 50cd4c6 into main Aug 24, 2021
@birkenfeld birkenfeld deleted the index_trait branch August 24, 2021 08:07
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.

None yet

2 participants