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

Fix Iter.{skip,take,nth} to check '.has_next()' of their inner iterator #2377

Merged
merged 2 commits into from
Nov 27, 2017

Conversation

mfelsche
Copy link
Contributor

Before this change Iter.take would continue iterating until n elements were taken or a call to next() of the inner iterator would error. has_next() has never been called.
The same is true for Iter.nth and Iter.skip.

While this works e.g. on Iterators over Array (i.e. ArrayValues) it does not for Iterators that continue returning elements in next() although has_next() already returned false (e.g. collections.Range).

@mfelsche mfelsche added changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge triggers release Major issue that when fixed, results in an "emergency" release labels Nov 26, 2017
@mfelsche mfelsche requested a review from Theodus November 26, 2017 19:04
fun ref has_next(): Bool => false
fun ref next(): U64^ => counter = counter + 1
end
)
Copy link
Contributor

Choose a reason for hiding this comment

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

The ) should be after the end keyword, like end), rather than on a new line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my bad. this is fixed.

@SeanTAllen
Copy link
Member

I want to hold on triggering a release until we are ready over at Wallaroo Labs. I'm working towards having our releases not be tied to ponyc's but I need one release that is coordinated first to do that.

@jemc
Copy link
Member

jemc commented Nov 27, 2017

@SeanTAllen - can we go ahead and merge this to get the fix on master, have this merge trigger the creation of a new release issue ticket, and move discussion into that ticket of waiting to actually do the release?

@mfelsche
Copy link
Contributor Author

I am fine either way. I just wanted to state that this would usually trigger a release.

@SeanTAllen
Copy link
Member

@jemc yes

@jemc jemc merged commit a1596e1 into master Nov 27, 2017
ponylang-main added a commit that referenced this pull request Nov 27, 2017
@jemc jemc deleted the itertools-iter-protocol branch November 27, 2017 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge triggers release Major issue that when fixed, results in an "emergency" release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants