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

Implement DoubleEndedIterator on Lines #22

Closed
wants to merge 2 commits into from
Closed

Implement DoubleEndedIterator on Lines #22

wants to merge 2 commits into from

Conversation

Avi-D-coder
Copy link

No description provided.

@cessen
Copy link
Owner

cessen commented Mar 7, 2019

@Avi-D-coder,
Thanks for the PR! However, a couple of things:

  1. This isn't an API that I want to support in Ropey. Eventually I plan to implement a cursor-style iterator for all of the existing iterators, which would get you roughly the same thing but far more useful for text processing. DoubleEndedIterator isn't really a good fit because of how limited it is. See issue Bidirectional iterators #18

  2. There are issues with your implementation that you should be aware of if you plan to maintain a branch of this for yourself. In particular, your code in reverse_line_to_byte_idx() doesn't handle CRLF line endings, which means it won't work properly with e.g. Windows text files.

@cessen cessen closed this Mar 7, 2019
@Avi-D-coder
Copy link
Author

@cessen Thanks for pointing that out.

I would request that you reconsider, waiting for rust-lang/rust#56345.
At ropey-iterator I added Clone, size_hint(), and ExactSizeIterator. With this combination Iterators gain most of the power of the Needle API, despite the suboptimal API.

In the long term you're definitely the Needle API will be a better fit, but last I checked it was blocked on specialization. Additionally since Lines does not leak implementation details the Iterators implementations can be changed to the Needle API without breaking compatibility.

@cessen
Copy link
Owner

cessen commented May 22, 2019

I would request that you reconsider, waiting for rust-lang/rust#56345.

Just to clarify, since this has come up again in issue #18, unless I am grossly misreading rust-lang/rust#56345, it has nothing to do with what I'm talking about here. As far as I know there is no proposal for a proper BidirectionalIterator trait or equivalent in Rust at the moment. Which is unfortunate.

As mentioned in my recent comment on issue #18, I think at this point I should bite the bullet and implement an appropriate API directly on Ropey's iterators. We can then trivially implement DoubleEndedIterator in terms of those if it's still considered useful at that point.

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