FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Implement str iterator support by mpajkowski · Pull Request #1171 · RustPython/RustPython · GitHub

  1. RustPython
  2. RustPython

Repository navigation

Implement str iterator support - #1171

Merged
coolreader18 merged 4 commits into
RustPython:masterfrom
mpajkowski:iter_str
Jul 24, 2019
ConversationCommits (4)ChecksFiles changed
Merged

Implement str iterator support#1171
coolreader18 merged 4 commits into
RustPython:masterfrom
mpajkowski:iter_str

Conversation

mpajkowski commented Jul 24, 2019

Copy link
Copy Markdown

Hello,

I added iteration support for str.

coolreader18 approved these changes Jul 24, 2019
View reviewed changes

coolreader18 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread vm/src/obj/objstr.rs
#[pymethod(name = "__iter__")]
fn iter(zelf: PyRef<Self>, _vm: &VirtualMachine) -> PyStringIterator {
PyStringIterator {
position: Cell::new(0),

coolreader18 Jul 24, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I believe you're missing the __reversed__ method somewhere here?

mpajkowski Jul 24, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Actually, I checked it using CPython (3.7.3):

>>> hasattr(str, "__reversed__")
False

Should I implement __reversed__ despite this fact or leave it unchanged?

coolreader18 Jul 24, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

How is PyStringReverseIterator used then?

mpajkowski Jul 24, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Within reverse:

string = "123456789"
reversed_iter = reversed(string)

coolreader18 Jul 24, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Right, but nowhere in your code do you construct a PyStringReverseIterator, so there's no way to actually get one in Python code.

mpajkowski Jul 24, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ok! I'll fix it in a moment.

mpajkowski commented Jul 24, 2019

Copy link
Copy Markdown
Author

Thanks for your feedback :) I'll deliver corrections today.

Marcin Pajkowski added 2 commits July 24, 2019 19:03
Add str.__reversed__ method
0e033ad
Silent clippy with allowing range_plus_one
1f59532 
PySliceableSequence trait methods require Range<usize>
as arguments

mpajkowski commented Jul 24, 2019

Copy link
Copy Markdown
Author

I corrected lint warning and added __reverse__ method - it seems to be an easiest solution.

coolreader18 merged commit a91470d into RustPython:master Jul 24, 2019
mpajkowski deleted the iter_str branch July 24, 2019 20:14
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

coolreader18 coolreader18 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL