-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Implement str iterator support #1171
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
Conversation
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.
This looks good, thank you for contributing to the project! I've made a comment on your changes and the CI is failing due to a linting error, but fix that up and this is good to merge.
#[pymethod(name = "__iter__")] | ||
fn iter(zelf: PyRef<Self>, _vm: &VirtualMachine) -> PyStringIterator { | ||
PyStringIterator { | ||
position: Cell::new(0), |
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 believe you're missing the __reversed__
method somewhere here?
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.
Actually, I checked it using CPython (3.7.3):
>>> hasattr(str, "__reversed__")
False
Should I implement __reversed__
despite this fact or leave it unchanged?
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.
How is PyStringReverseIterator
used then?
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.
Within reverse
:
string = "123456789"
reversed_iter = reversed(string)
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.
Right, but nowhere in your code do you construct a PyStringReverseIterator
, so there's no way to actually get one in Python code.
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.
Ok! I'll fix it in a moment.
Thanks for your feedback :) I'll deliver corrections today. |
PySliceableSequence trait methods require Range<usize> as arguments
I corrected lint warning and added |
Hello,
I added iteration support for
str
.