-
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
Add numeric step kw args #3356
Add numeric step kw args #3356
Conversation
With #3320 being merged, could you rebase according to the master branch? That should get rid of the enum commits from this PR. |
b5fc9bc
to
6031865
Compare
Rebase done! |
Looking at this, is this based on recent changes in MRI? If so, is there any link to the commit(s)/NEWS file detailing these changes? |
|
||
describe :numeric_step, :shared => true do | ||
|
||
def get_args(*args) |
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'm not sure if this was already present before (bit hard to grok based on the diff), but if not then this should be moved into a fixture file of sorts. Doing so keeps the spec files clean.
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.
The get_args method is new and it's specific to step method specs with kw arguments. It allows to run the tests with different argument passing strategies.
My idea would be to move it into a into a specific helper file. What do you think?
This is based on MRI 2.1. |
6031865
to
7831e82
Compare
Hi @yorickpeterse, Moved the get_args method to a helper file, but I'm still not sure if it's the best place for it. Thanks |
@fmfdias We might not need it at all to begin with, but I'll have to dig through the specs myself to see if that is the case. I'll try to take a look at it this weekend. |
Ok :) Let me know of any changes needed! |
end | ||
end | ||
|
||
raise ArgumentError, "step cannot be 0" if step == 0 |
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 error is still raised in case of the following:
10.step(100, 0).to_a => ArgumentError: step can't be 0
Is this now taken care of somewhere 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.
The logic moved to the mirror since to allow it to be shared with the part where the enumerator size is calculated.
You can see it in Rubinius::Mirror::Numeric#step_fetch_args.
Regarding some of the comments I made: I know part of these specs were copied over from the existing file, but I think now is a good time to clean them up properly. In general these specs really need to be broken up into separate |
Thanks for the review :) About the spec cleaning, I'll gladly help clearing out this specs. Thank you! |
7831e82
to
94ea394
Compare
Hi @yorickpeterse, Started the spec cleanup/reorganization.
In terms of the "step as string" argument error specs, I think the idea behind the tests that are being made is to check that even when the strings are numeric representations, the exception is raise. Having this is mind I reorganized them in a slightly different way, but if you think they don't make send, I'll act accordingly :) While doing this I fixed some specs that could result in false positives (they were using == instead of eql) and fixed one issue that wasn't being caught because of this. About the ruby_bug guards, should I remove them too? Don't worry about the commits, I'll rebase this in the end. |
Yeah, they can be removed. I'll take a closer look at the rest this weekend. |
94ea394
to
df28f06
Compare
Having taken another look, I still stand by having to somehow refactor (out) |
df28f06
to
c8be76f
Compare
Hi @yorickpeterse, Sorry about the late reply. Unfortunately I'm not having too much time lately. Today I had a little, so I tried to come back to this and try a different approach to the last standing issue :) Instead of using a helper method as a way help share specs between different argument passing types, the shared spec now requires a variable with a Proc/lambda to transform the arguments to the desired argument passing type. While doing this, I also synced master changes into the branch. Thanks! |
I'll take a look at this somewhere this week. One thing I wonder: do we have to duplicate tests for both positional and keyword argument forms? Can we perhaps not write the tests for positional arguments and then a few simple ones to check if keyword arguments are also used? I'm not sure if this would be more rigid, but it would probably simplify the sharing of specs a bit. |
c8be76f
to
415f0fe
Compare
Hi @yorickpeterse, Thanks! Note: When using keyword arguments the method accept 0 as step, while the positional argument style doesn't. That's why I placed the same between quotes when I said the behavior of the method is the same. Thank you! |
415f0fe
to
aa8568c
Compare
34a4d79
to
8cbd36d
Compare
The implementation moves the specs that applies to all modes (positional arguments, keyword arguments and mixed arguments) to a shared spec file. The specific code is maintained on the main Numeric#step spec file.
When one the params is a float, the results are floats.
8cbd36d
to
bd606b5
Compare
@fmfdias sorry it took so long to merge this. Thanks for the help! |
Hi guys,
This PR adds support for keyword arguments on Numeric#step method.
Since this method spec and implementation were changed during the implementation of #3320, these changes were made over that to avoid conflicts. Because of this, this PR should only be merged after #3320.
Thank you!