search

cessen / ropey

A utf8 text rope for manipulating and editing large texts.
MIT License
1.04k stars 46 forks source link

Implement DoubleEndedIterator on Lines #22

Closed Avi-D-coder closed 5 years ago

cessen commented 5 years ago

@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 #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.

Avi-D-coder commented 5 years ago

@cessen Thanks for pointing that out.

I would request that you reconsider, waiting for https://github.com/rust-lang/rust/issues/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 commented 5 years ago

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 https://github.com/rust-lang/rust/issues/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.