-
Notifications
You must be signed in to change notification settings - Fork 778
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: add slice indexing #1829
Conversation
cd83a91
to
b16cf66
Compare
Thanks for the great review! I reworked this to be a macro as suggested. (I threw it in I also made conditional use of e.g.
|
b16cf66
to
21759b9
Compare
src/types/sequence.rs
Outdated
} | ||
|
||
index_impls!(PySequence, PyAny, "sequence", sequence_len, sequence_slice); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that the output of get_slice
is currently not another PySequence
, should we change this? IMO it's similar to expecting a sequence out of repeat
etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think that's reasonable.
I guess as before, implementations may actually not return a sequence. Then using further sequence APIs will fail, which is ok and sound.
Very nice (also that we can do it conditionally)! |
21759b9
to
3a90ff7
Compare
Follow up to #1825 to add range indexing.
Given that the decision in #1733 was that we are trying to make these APIs feel as much like idiomatic Rust as possible, I opted to make them panic on out of bounds.
This probably closes #1667 ?