Skip to content

Reimplements Range#first #3261

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

Merged
merged 1 commit into from
Nov 22, 2016
Merged

Reimplements Range#first #3261

merged 1 commit into from
Nov 22, 2016

Conversation

ksss
Copy link
Contributor

@ksss ksss commented Nov 21, 2016

Range#first with argument works like Enumerable#take.
Range#first shouldn't call Range#to_a on infinity range.

$ mirb
mirb - Embeddable Interactive Ruby Shell

> (0..Float::INFINITY).first(3)
# ...infinite loop...

So, I reimplemented Range#first with Ruby code.
Because it hard to write break in iterator on C functions.
And this implementation passed specs of that https://github.com/ruby/spec/blob/66daf290cb49707190481b4cf24e700010cd53bf/core/range/first_spec.rb
(I know that it's a bit too strictly)

Range#first shouldn't call `Range#to_a` on infinity range.

raise ArgumentError, "wrong number of arguments (given #{args.length}, expected 1)" unless args.length == 1
nv = args[0]
raise TypeError, "no implicit conversion from nil to integer" unless nv.nil?.!
Copy link
Contributor

Choose a reason for hiding this comment

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

We could simplify that to if nv.nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just wanted to unify unless with raise.

@matz matz merged commit bbaf3c2 into mruby:master Nov 22, 2016
matz added a commit that referenced this pull request Nov 22, 2016
@ksss ksss deleted the range-first branch November 22, 2016 08:39
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