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

Support Enumerable#each_with_index #9

Closed
wants to merge 2 commits into from
Closed

Support Enumerable#each_with_index #9

wants to merge 2 commits into from

Conversation

amatsuda
Copy link
Contributor

It used to work correctly, but does not work in recent gems since 45435b3.

I suppose this causes SyntaxError on Ruby < 1.9, but do we really still have to support Ruby 1.8?
I'm sorry I couldn't come up with any good solution to support both each_with_index and Ruby 1.8. So, I'd propose dropping Ruby 1.8 support, and bring each_with_index support back.

@toy
Copy link
Owner

toy commented Sep 30, 2017

Thanks for reporting.
I see that the fix I did in 45435b3 is no good, https://bugs.ruby-lang.org/issues/6994 describes the difference in 1.8 behaviour of yield with splat arguments.
Problem can be fixed for all rubies by using block.call instead of yield, but rubocop points to performance penalty: https://github.com/JuanitoFatas/fast-ruby#proccall-and-block-arguments-vs-yieldcode, not sure if it will really be noticeable with some work done in the block.
Or different methods can be used for Ruby < 1.9 and all others. I am really not sure when to drop Ruby 1.8 support, statistics on ruby versions usage would probably help a lot like requested in rubygems/rubygems.org#1439

@amatsuda
Copy link
Contributor Author

amatsuda commented Oct 5, 2017

@toy You're right! block.call can support both 1.8 and each_with_index. I made another PR #10 with that change.

Regarding Ruby 1.8, it was EOFed 4 years ago https://www.ruby-lang.org/en/news/2013/06/30/we-retire-1-8-7/, and so it's strongly discouraged to use.
So you don't have to care about 1.8 at all IMO. There still can be an existing 1.8 environment, but I don't think there's a real user who are eager to update a gem library's version but doesn't want to update their Ruby version.

If I were a maintainer of this gem I would take #9, but if you prefer #10's approach, that would be also fine.

@toy
Copy link
Owner

toy commented Oct 8, 2017

Probably you are right. I'll go with releasing two versions: with #10 as last version with fix for 1.8 and #9 as first version without 1.8

@toy
Copy link
Owner

toy commented Oct 8, 2017

Merged in fc4354c

@toy toy closed this Oct 8, 2017
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.

None yet

2 participants