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

Deprecate reverse_iter to Iterators.reverse #670

Merged
merged 1 commit into from
Sep 4, 2020

Conversation

nickrobinson251
Copy link
Member

@nickrobinson251 nickrobinson251 commented Aug 30, 2020

@oxinabox
Copy link
Member

oxinabox commented Aug 31, 2020

Seems like this is inline with the documentation for Base.Iterators.reverse

help?> Base.Iterators.reverse
  Iterators.reverse(itr)

  Given an iterator itr, then reverse(itr) is an iterator over the same collection but in the
  reverse order.

  This iterator is "lazy" in that it does not make a copy of the collection in order to reverse
  it; see Base.reverse for an eager implementation.

  Not all iterator types T support reverse-order iteration. If T doesn't, then iterating over
  Iterators.reverse(itr::T) will throw a MethodError because of the missing iterate methods for
  Iterators.Reverse{T}. (To implement these methods, the original iterator itr::T can be obtained
  from r = Iterators.reverse(itr) by r.itr.)

Iterators.reverse was implemented by @stevengj in JuliaLang/julia#24187
perhaps he can comment on if it is allowable to return something other than a Reverse{T}

I guess the question is particular for Stack which for its reverse would iterate reverse(reverse(::Deque)
so it makes sense to have it just iterate the Deque.
https://github.com/JuliaCollections/DataStructures.jl/pull/670/files#diff-7356e908345ca352e9d4e26734784cfeR56

src/deque.jl Outdated Show resolved Hide resolved
Copy link
Member

@oxinabox oxinabox left a comment

Choose a reason for hiding this comment

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

I think this is a good idea.
Lets give it a few days for others to chime in, and if noone says it is a bad idea lets merge it,
after bumping the patch version

@nickrobinson251
Copy link
Member Author

nickrobinson251 commented Aug 31, 2020

the question is particular for Stack which for its reverse would iterate reverse(reverse(::Deque)
so it makes sense to have it just iterate the Deque.
https://github.com/JuliaCollections/DataStructures.jl/pull/670/files#diff-7356e908345ca352e9d4e26734784cfeR56

Yeah. I wonder: even if it's okay for Iterators.reverse(::Stack) to return DequeIterator (rather than a Reverse{<:Stack}), perhaps the return should still implement all the methods that work on Iterators.Reverse, such as first and last? We'd need to add last for DequeIterator i think.

@stevengj
Copy link

perhaps he can comment on if it is allowable to return something other than a Reverse{T}

Absolutely. That's why it's called reverse(itr) and not Reverse(itr). For example, we already do:

julia> Iterators.reverse(1:10)
10:-1:1

Update src/deque.jl

Implement `last` for `Stack` and `reverse(::Stack)`

Bump patch version

Test `length` for `reverse`
@nickrobinson251
Copy link
Member Author

^ just rebased to resolve merge conflicts

@oxinabox oxinabox changed the title RFC: Deprecate reverse_iter to Iterators.reverse Deprecate reverse_iter to Iterators.reverse Sep 4, 2020
@oxinabox
Copy link
Member

oxinabox commented Sep 4, 2020

Merge and tag when ready.

@nickrobinson251
Copy link
Member Author

i don't have permissions to do that. But feel free to give me them if you want. Or please merge and tag when you've the time

@oxinabox
Copy link
Member

oxinabox commented Sep 4, 2020

I gave you permissions several days ago.
I see you are still listed as pending accepting.
I think you can go to https://github.com/orgs/JuliaCollections/people
to accept.
or if not you'll need to find the email

@nickrobinson251
Copy link
Member Author

ah, nice, thanks! i missed that email (i get too many GitHub email and need to fix that)
But i could see the invite by jsut going to https://github.com/JuliaCollections :)

@nickrobinson251 nickrobinson251 merged commit 897c333 into JuliaCollections:master Sep 4, 2020
@nickrobinson251 nickrobinson251 deleted the npr/reverse-iter branch September 4, 2020 15:39
@nickrobinson251 nickrobinson251 mentioned this pull request Sep 4, 2020
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.

3 participants