-
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
Fix enumerator size #3320
Fix enumerator size #3320
Conversation
@@ -351,7 +351,18 @@ def any? | |||
end | |||
|
|||
def cycle(many=nil) | |||
return to_enum(:cycle, many) unless block_given? | |||
unless block_given? | |||
size = Proc.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.
This pattern occurs in a few places in this PR so this comment applies to all of them: can we not simply pass the size itself here instead of a Proc? There's little overhead in calculating the size at this point and it removes the need for evaluating the block on every #size
call later on.
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.
I agree with you! These sizes are not that complex to calculate. There will be others, like the Array#combination that might be worthwhile calculating it only if needed. Will see when we get there, but in the meantime I'll change these.
Spec changes look fine to me. For Enumerable/Enumerator we might want to do some general book keeping to clean things up, especially regarding passing in enum sizes. |
To further iterate (now that I'm at the office): I think we should fix
This would still match the signature of MRI (which is |
Hi @yorickpeterse, Thanks for the input. I'm doing this off hours, and with some time constraints. That's why I'm only replying now. My main objective is to help and so because I like this, but if you guys feel that the time might be an issue, please go ahead and continue with it as you see better. I'll continue to help in other ways. Will get back when I have more news. |
There's no rush, so take your time :) |
Hi @yorickpeterse, Thanks!
Doing this would greatly reduce the amount of work of creating/changing the specs. I was thinking on using it_behaves_like but I don't if it's the best way to do it. There's also the question of where to place in the case of having to share them with Enumerables like Array. I'll try to figure it out, but any advice would be great. Thank you |
For re-using specs we use shared examples, e.g. https://github.com/rubinius/rubinius/blob/master/spec/ruby/shared/enumerator/with_object.rb. This can then be used as following: https://github.com/rubinius/rubinius/blob/master/spec/ruby/core/enumerator/with_object_spec.rb#L5. This is probably the best solution here since Array and Enumerable share the same expected behaviour. Regarding the patterns, make sure the changes also take care of lazy enumerators as in that case |
I had a failing spec I forgot to add before for each_with_index_spec: it "has correct size with no proc given" do
[1, 2, 3, 5].each_with_index.size.should == 4
end @fmfdias I thought I'd mention it in case you want to add it. |
Hi guys, Thanks for the feedback. I've updated the specs with two shared spec files. The enumeratorized is to be use by any enumerable implementation and besides implementing the 2 first use cases I enumerated in my last comment, it also has the cycle case that's shared with the Array. The enumerable_enumeratorized is specific to the Enumerable specs and basically adds tests for Enumerable with no size method implementation. The usage of this specs relies on it_behaves_like on most cases and in some edge cases it relies on the it_should_behave_like (e.g: Other shared specs, each_slice, each_cons...) @yorickpeterse Thanks for the ideas. I also checked with MRI 2.1.5 and size is also passed to lazy enumerators. This version also does that :) @bjfish Thank you, this version also has a spec for each_with_index. If you remember any other that might be missing, please tell me and I'll add. For next steps I'm going to add some missing specs on Array (e.g: combination, permutation, etc...) and start implementing them. Thanks |
Hi, Added the implementation for the Array methods that were failing the specs. I moved to Hash... added failing specs and started to implement it on hash.rb when I found out that there's another hash implementation (hash_hamt.rb). Thank you |
@fmfdias |
@jemc Thank you! The Hash part is implemented. Moving to Range! Cheers |
Hi guys, A little status update... Have a nice weekend! |
Thanks for looking into this, and take your time! 👍 |
Hi guys, @yorickpeterse: Thanks :) I think the bulk work is done. I tried to find, check and fix every Kernel#to_enum, Kernel#enum_for and Enumerator#new existing on the Rubinius code. Hope none has skipped, hehehe. Please review and I'll make the necessary changes as soon as I can. Have a nice weekend! |
I haven't looked at the actual code yet (I'll do so in the next few days), but looking at the commits themselves I have two nit-picky comments:
Both these changes can be made using an interactive Git rebase ( |
return 0 if comb < 0 | ||
__descending_factorial__(size, comb) / __descending_factorial__(comb, comb) | ||
end | ||
private :__binomial_coefficient__ |
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.
This should probably be moved into a mirror for the Array class (https://github.com/rubinius/rubinius/blob/master/kernel/bootstrap/array_mirror.rb), that way we don't pollute the Array class nor can Ruby code in the wild mess with it. Mirrors can be used as following: https://github.com/rubinius/rubinius/blob/master/kernel/common/array.rb#L48. Another benefit of using a mirror is that this removes the need for the ugly __
prefix/suffix.
After looking through the code I have two general remarks:
|
…eturn an Enumerator instance.
…n an Enumerator instance.
… an Enumerator instance.
…urn an Enumerator.
…turn an Enumerator instance.
…urn an Enumerator instance.
…t return an Enumerator instance.
… return an Enumerator instance.
8f509ff
to
2d239ba
Compare
Hi @yorickpeterse, Thanks for the clarifications! I ran the specs with ruby 2.2.1 and they also pass, so I think this work can also be reflected on 2.2 branch too. Ruby 2.2 brought two new Enumerable methods, slice_before and slice_when. Thank you! |
@fmfdias Thanks for all of this work, and @yorickpeterse thanks for coaching it. Rubinius is so exciting, and it's magical to watch the community working together so effectively. |
Hi @tmornini and @yorickpeterse, Thanks 😊 I'm really glad I could to help! It's been a great experience and I'm enjoying it a lot. 😃 @yorickpeterse I'll try to do any change request as soon as I can so this one can be released. Speaking of the #3269, is/was there any similar issue opened for Ruby 2.0/2.1? I'm asking this because while developing this one I found out that on MRI > 2.1, Numeric#step supports keyword arguments. There might be others in similar situation, so I was thinking on checking for this changes that might be missing from Rubinius. You guys have a nice weekend! |
Not sure if I'm following. If you're referring to other changes required for 2.2 compatibility, sure, there's probably stuff not mentioned in the NEWS file and the corresponding issue. |
Sorry... wrong ticket 😅 The one I wanted to mention was #3264. Having a check list is cool to check how much specs are still missing. I was trying to know if there was a similar one for 2.0 and 2.1 compatibility. If they exist, my idea is to compare them to the NEWS file and spot any other missing stuff, like the Numeric#step keyword arguments. Thanks |
As far as I'm aware there's no ticket for what remains to be added for 2.0/2.1. |
Thanks! I'll try to do a quick check to see if anything more is missing. Thank you! |
@fmfdias Doing that in a separate PR is probably best (keeps things clear). You can simply branch said PR from this one which should prevent any merge conflicts. |
Sorry for the delay, but didn't had time to finish the Numeric#step implementation before. Thank you! |
Looking into this at the moment, in particular:
Otherwise things are looking fine, I should be done with this in 30 minutes or so :) |
Cool! Sorry about the commit message. I'll take that into consideration next time I commit to the project :) Thank you |
The Enum stuff has been rebased into the following commits: ac5f92a...64d73f9 The String#sub changes have been rebased into the 2.2 branch as the following commits: ccf1686...33dae72 |
The commits in ccf1686...33dae72 mentioned things changed in Ruby 2.2. Since we're dropping the use of version guards this means that until we fully support 2.2 changes for said version have to go in a separate branch. Or do the commits mean that this has changed since 2.1 (and not 2.2)? |
Ah I see, this changed in 2.1 and not 2.2. In that case I'll merge the commits into the master branch as well. |
With this merged/sorted out I'm closing this. Big props for taking care of this! 👍 🍺 |
That behavior comes at least from 2.0. |
@yorickpeterse |
Hi,
This is an attempt to fix the Enumerator#size method issues identified previously here and here. It's still a work in progress but I opened this pull request to check with you guys if I'm on the right track and also get some guidance.
I took a look at the JRuby implementation and what they do is pass the a 'calculated' size to the enumerator. Usually the size is enough (each, reverse_each, etc...), but there are some methods that need some math, like the cycle, each_cons and each_slice.
To start I've implemented a to_enum_with_size private method on the Enumerable module to use on these methods to create the an enum with the size calculation. Doing it here allow it to be used on Array and other classes that mix in the Enumerable module. Nevertheless, my question here is if it's the right place to do it.
Another thing I would like to point out is the each_cons and each_slice size spec. The size calculation is a little bit more complex that the cycle. The spec is only testing some value combinations, but I think it could be better. Do you guys have a better way to implement it?
When I was at it, I saw that both each_cons and each_slice raise an exception when called without a block. I added the calls to the spec that tests that the exception is raised and fixed it.
Like I said, this is still a work in progress. Enumerable seems to be fixed with these changes, but Array and other aren't. I already started with Array. With time, and if you are all ok with these changes, I'll continue to add them to this pull request.
Thank you