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 chunk_while #3579

Merged
merged 5 commits into from Jan 20, 2016
Merged

Add chunk_while #3579

merged 5 commits into from Jan 20, 2016

Conversation

@jsyeo
Copy link
Member

@jsyeo jsyeo commented Jan 20, 2016

No description provided.

@@ -212,6 +212,12 @@ def slice_when(&block)
end
end

def chunk_while(&block)
raise ArgumentError, "wrong number of arguments (0 for 1)" unless block_given?
Copy link
Member

@jemc jemc Jan 20, 2016

Choose a reason for hiding this comment

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

This doesn't strike me as a particularly accurate or helpful error message.

Even if it's the same as the one given by MRI, I'd prefer to see it made clear that a block is required. The current error message makes it look like a regular argument is required.

Loading

Copy link
Member

@brixen brixen Jan 20, 2016

Choose a reason for hiding this comment

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

Hm, interesting that this doesn't return an Enumerator when no block is given. I see this in the 2.3.0 ri chunk_while docs:

= .chunk_while

(from ruby core)
=== Implementation from Enumerable
------------------------------------------------------------------------------
  enum.chunk_while {|elt_before, elt_after| bool } -> an_enumerator

------------------------------------------------------------------------------

Assuming this should raise an ArgumentError, I agree with @jemc here, we should use a more helpful error message.

Loading

Copy link
Member

@brixen brixen Jan 20, 2016

Choose a reason for hiding this comment

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

Ah, I see now why the block is required. So yeah, we should fix the error message.

Loading

Copy link
Member Author

@jsyeo jsyeo Jan 20, 2016

Choose a reason for hiding this comment

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

What should the error be? How about "Block required"?

Loading

Copy link
Member Author

@jsyeo jsyeo Jan 20, 2016

Choose a reason for hiding this comment

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

Or how about "no block given"? For consistency with chunk in https://github.com/rubinius/rubinius/blob/master/kernel/common/enumerable.rb#L8

Loading

Copy link
Member

@jemc jemc Jan 20, 2016

Choose a reason for hiding this comment

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

👍

Loading

@jsyeo
Copy link
Member Author

@jsyeo jsyeo commented Jan 20, 2016

Changed error message to "no block given".

Loading

def chunk_while(&block)
raise ArgumentError, "no block given" unless block_given?

slice_when { |before, after| !(block.call(before, after)) }
Copy link
Member

@YorickPeterse YorickPeterse Jan 20, 2016

Choose a reason for hiding this comment

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

The explicit block isn't needed here, this can just be:

!(yield before, after)

Loading

@brixen
Copy link
Member

@brixen brixen commented Jan 20, 2016

Thanks!

Loading

brixen added a commit that referenced this issue Jan 20, 2016
@brixen brixen merged commit 5d7d24a into rubinius:master Jan 20, 2016
1 check passed
Loading
@chuckremes chuckremes mentioned this pull request Jan 20, 2016
52 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants