-
Notifications
You must be signed in to change notification settings - Fork 605
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
Trying to fix #3306 each_slice #3315
Conversation
Tried to understand enum.c and produce something similar where a size function can be specific.
@size | ||
elsif @size.respond_to? :size | ||
@size.size | ||
else |
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.
What is the reason for changing this method itself? As far as I'm aware @size
is always either a Numeric or a Proc, thus it will never respond to size
. Checking if it's a Numeric
also isn't really needed since we can guarantee the types of @size
.
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.
@yorickpeterse in the case of each_with_index it was returning the array, there may be another (more correct) way to resolve this
[1, 2, 3, 5].each_with_index.size
-> [1, 2, 3, 5]
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.
You mean size
was returning an Array? If so, that should probably belong in its own PR (unless it's directly related to each_slice
).
The core problem of this bug is that From the top of my head, an easy way to probably fix this would be to simply use the following: def size
if @size.kind_of?(Proc)
@size.call
elsif @size
@size
else
count
end
end Since |
@@ -56,4 +56,10 @@ | |||
multi = EnumerableSpecs::YieldsMulti.new | |||
multi.each_slice(2).to_a.should == [[[1, 2], [3, 4, 5]], [[6, 7, 8, 9]]] | |||
end | |||
|
|||
it "has correct size with no proc given" 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.
Spec descriptions should read as natural English. While quite a few existing descriptions don't follow this guideline we try to apply it to new changes. As such I'd suggest writing this something along the lines of it "returns the correct size when no block is given"
. The same applies to the each_with_index
spec.
Updating spec name and method name per code review
Tried to understand enum.c to fix #3306 and produce something similar where a size
function can be specified.
The size functions there are a little difficult to follow and the enum library here might need more refactoring in this area. This is just a taking a shot at fixing the issue without causing regression.
Forgive me for not reading all the contributing guidelines yet but I did run all the specs (I think).