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

Trying to fix #3306 each_slice #3315

Merged
merged 3 commits into from
Feb 13, 2015
Merged

Conversation

bjfish
Copy link
Contributor

@bjfish bjfish commented Feb 12, 2015

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).

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
Copy link
Contributor

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.

Copy link
Contributor Author

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]

Copy link
Contributor

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).

@yorickpeterse
Copy link
Contributor

The core problem of this bug is that @size is set to nil when using Enumerator#each_slice. Because of this the @size.kind_of?(Proc) check evaluates to false, leading to the #size method just returning @size (and thus nil).

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 Enumerator#count does work properly on an Enumerator returned by each_slice this will most likely do the trick.

@@ -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
Copy link
Contributor

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.

@bjfish bjfish changed the title Trying to fix #3306 each_slice and each_with_index Trying to fix #3306 each_slice Feb 13, 2015
jemc added a commit that referenced this pull request Feb 13, 2015
@jemc jemc merged commit 17172bf into rubinius:master Feb 13, 2015
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.

Mismatch on Enumerator#size result between Rubinius and MRI
3 participants