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

PeekingNext has a somewhat strange API #679

Open
ijackson opened this issue Feb 28, 2023 · 1 comment
Open

PeekingNext has a somewhat strange API #679

ijackson opened this issue Feb 28, 2023 · 1 comment

Comments

@ijackson
Copy link

peeking_next(&mut self, accept: impl FnOnce(..) ->bool) can be implemented in terms of peek(&mut self) -> Option<&Item> and next. Conversely, peek can be implemented in terms of peeking_next. So, they are equivalent.

However, peeking_next is a more complicated API. So IMO ideally,

  • The trait's required method ought to be peek. (This has a name clash with std::iter::Peekable::peek, but I think that's OK because the inherent method will take priority when the type is just Peekable.)
  • peeking_next ought to be a provioded method.
  • Probably, then, the name is then wrong. Peek seems like it would be right.

These would be breaking changes, but I think implementations of PeekingNext are going to be tolerably rare, and the fix will be obvious. The key thing will be to avoid breaking all trait bounds. I suggest, in a maor revision,

  • Abolish PeekingNext and replace it with Peek, with peek as required method and peeking_next as provided.
  • Additionally export Peek as PeekingNext with a #[deprecated].
  • Possibly provide #[deprecated] pub fn impl_peek_via_peeking_next(self_: &mut impl Peek, accept: ...) -> ...
  • Put appropriate notes in the docs about the migration.

I think the effect is that:

  • Downstreams that don't impl PeekingNext will see no actual break, although they'll see a new deprecation warning.
  • Downstreams that impl PeekingNext will get an error saying "you need to implement the required method peek". Turning an implementation of peeking_next into an implementation of peek will almost always be easy, just deleting code. Alternatively, the downstream could just add fn peek(&mut self) -> ... { itertools::impl_peek_via_peeking_next..., if we chose to provide it.

This should probably be done at the same time as #678, which becomes impl Peek for &mut Peek.

@jendrikw
Copy link

jendrikw commented Jul 9, 2023

See also #583

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

No branches or pull requests

2 participants