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

Add NCycle iterator #17935

Closed
wants to merge 1 commit into from
Closed

Add NCycle iterator #17935

wants to merge 1 commit into from

Conversation

lstagner
Copy link
Contributor

I think it is useful to be able to set how many times an iterator should be cycled over.

julia> for i=cycle(1:2,2)
           println(i)
       end
1
2
1
2

The documentation should be changed but I am unsure of where to change it.

@tkelman tkelman added the needs tests Unit tests are required for this change label Aug 10, 2016
@tkelman
Copy link
Contributor

tkelman commented Aug 10, 2016

The docstring for this is currently at

julia/base/docs/helpdb/Base.jl

Lines 5565 to 5570 in 3f6b2b2

"""
cycle(iter)
An iterator that cycles through `iter` forever.
"""
cycle
, but this would be a good opportunity to move it inline next to the source code. If you change the signature in the docstring, you should make the corresponding change to the signature in doc/stdlib/collections.rst then run make docs so the content of the docstring gets translated over.

@tkelman tkelman removed the needs tests Unit tests are required for this change label Aug 10, 2016
@lstagner
Copy link
Contributor Author

I have added tests and documentation. I have also moved the cycle docs inline as you suggested.

@JeffBezanson
Copy link
Member

This doesn't work as well as it could since it requires xs to support length.

@lstagner
Copy link
Contributor Author

Do you have a suggestion on how to improve it? The only way I know of is to create a new iterator type.

@kshyatt
Copy link
Contributor

kshyatt commented Aug 10, 2016

Well, well, this is your first Julia PR, isn't it @lstagner? Welcome to Julia!

If you could run the doctests as well make -C doc doctest to make sure they all still pass, that would be awesome.

@lstagner
Copy link
Contributor Author

I have done a rebase against master and all the unrelated doc test failures now pass.

This is ready to merge whenever.

@lstagner lstagner changed the title Add optional argument to cycle iterator to set the number of cycles Add NCycle iterator Aug 30, 2016
@lstagner
Copy link
Contributor Author

This PR wasn't getting much traction so I decided to create a custom iterator type instead of using take.

julia> for i=ncycle(1:2,2)
           println(i)
       end
1
2
1
2

@lstagner
Copy link
Contributor Author

I have rebased and this is ready for review/merging.

I addressed @JeffBezanson comment by making a new iterator type NCycle instead of using take.

@iamed2
Copy link
Contributor

iamed2 commented Oct 16, 2016

As a new iterator this might fit better in Iterators.jl.

@JeffBezanson
Copy link
Member

Are there use cases for this that justify putting it in Base? The existing cycle iterator itself doesn't seem to be used in Base, and with #5155 these things are drawing closer scrutiny.

@lstagner
Copy link
Contributor Author

If you think Iterators.jl is a better fit for this I will open up a PR there.

@lstagner
Copy link
Contributor Author

@lstagner lstagner closed this Dec 14, 2016
@lstagner lstagner deleted the ls/cyclen branch December 14, 2016 02:23
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.

5 participants