-
Notifications
You must be signed in to change notification settings - Fork 612
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
Implement Enumerable#slice_after #3351
Conversation
raise ArgumentError, "wrong number of arguments (0 for 1)" | ||
when [true, false] | ||
block = Proc.new { |elem| pattern === elem } | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might I suggest the following instead:
if pattern_given && block_given?
raise ArgumentError, "cannot pass both pattern and block"
elsif pattern_given
block = Proc.new { |elem| pattern === elem }
elsif block_given?
# use given block
else
raise ArgumentError, "wrong number of arguments (0 for 1)"
end
I think we should avoid allocating a bunch of unnecessary Arrays if we can (it seems like it would be a performance hit), and I think my example is still just as readable, if not a little more so. The example I gave could be collapsed a bit more into nested if
s, but I care less about that difference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright. How about this:
raise ArgumentError, "cannot pass both pattern and block" if pattern_given && block_given?
raise ArgumentError, "wrong number of arguments (0 for 1)" if !pattern_given && !block_given?
block = Proc.new { |elem| pattern === elem } if pattern_given
# rest of code
@@ -160,6 +160,28 @@ def slice_before(arg = undefined, &block) | |||
end | |||
end | |||
|
|||
def slice_after(pattern = undefined, &block) | |||
pattern_given = !(undefined.equal? pattern) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!undefined.equal?(pattern)
is more natural.
10f8afd
to
0cf5e0b
Compare
pattern_given = !undefined.equal?(pattern) | ||
raise ArgumentError, "cannot pass both pattern and block" if pattern_given && block_given? | ||
raise ArgumentError, "wrong number of arguments (0 for 1)" if !pattern_given && !block_given? | ||
block = Proc.new { |elem| pattern === elem } if pattern_given |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably should've been more clear with this (my bad), I meant whitespace between every line, not just the one preceding Enumerator.new do ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh... okay got it. 👍
@yorickpeterse I've added the newlines. 😄 |
Now that we have the specs for Enumerable#slice_after, we can implement it!
I hope I didn't confuse anyone with the crazy case when usage. Correct me if I am wrong, but I think it's more readable than a bunch of
if
s andnil?
s.