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

Fixed yielded arrays of 1 element bug when using Enumerator#first #2458

Merged
merged 1 commit into from Jan 14, 2015

Conversation

Who828
Copy link
Contributor

@Who828 Who828 commented Jan 13, 2015

Fixes an old issue #2376 with RubyYielder class.

This fix passes Rubyspecs and MRI specs, however let me know if there any changes required.

@headius
Copy link
Member

@headius headius commented Jan 14, 2015

Good to get this fixed, but I hope we can figure out a lighter way to do it in the future. If I understand the logic, we're basically double-wrapping the value so that when unwrapped by yield logic it comes out unharmed. Will merge today.

headius added a commit that referenced this issue Jan 14, 2015
Fixed yielded arrays of 1 element bug when using Enumerator#first
@headius headius merged commit 0b27dbc into jruby:master Jan 14, 2015
1 check passed
@headius headius added this to the JRuby 9.0.0.0-pre1 milestone Jan 14, 2015
@headius headius self-assigned this Jan 14, 2015
nightscape
Copy link
Contributor

@nightscape nightscape commented on cb25efe Apr 29, 2016

Choose a reason for hiding this comment

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

Can anyone remember why this change was introduced? It seems to break the expected behavior:
#3814

headius
Copy link
Member

@headius headius commented on cb25efe Apr 29, 2016

Choose a reason for hiding this comment

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

There have been ongoing bugs with Enumerable/Enumerator yielding the proper expanded/unexpanded arrays of args...I don't remember which test/behavior this was supposed to fix.

nightscape
Copy link
Contributor

@nightscape nightscape commented on cb25efe May 2, 2016

Choose a reason for hiding this comment

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

@Who828 Do you remember by any chance what this change intended to fix? It causes other bugs and I would like to create specs that affirm that all known issues are solved.

@nightscape
Copy link
Contributor

@nightscape nightscape commented May 2, 2016

Ah, ok, so this is the corresponding PR. Sorry, didn't find it at first (hint: you can search for a SHA in Github and it will find the corresponding PR).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants