Add numeric step kw args #3356

fmfdias · 2015-03-12T21:36:57Z

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!

yorickpeterse · 2015-03-12T23:13:06Z

With #3320 being merged, could you rebase according to the master branch? That should get rid of the enum commits from this PR.

fmfdias · 2015-03-12T23:19:43Z

Rebase done!

yorickpeterse · 2015-03-12T23:22:55Z

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?

yorickpeterse · 2015-03-12T23:25:25Z

spec/ruby/core/numeric/shared/step.rb

+
+describe :numeric_step, :shared => true do
+
+  def get_args(*args)


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.

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?

fmfdias · 2015-03-12T23:32:56Z

This is based on MRI 2.1.
Here's the NEWS lines https://github.com/ruby/ruby/blob/v2_1_0/NEWS#L102-L106

fmfdias · 2015-03-13T01:13:39Z

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.
Should it go to the numeric helper on mspec?

Thanks

yorickpeterse · 2015-03-13T09:51:07Z

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

fmfdias · 2015-03-13T22:57:01Z

Ok :) Let me know of any changes needed!

yorickpeterse · 2015-03-15T12:05:39Z

kernel/common/numeric.rb

      end
    end

-    raise ArgumentError, "step cannot be 0" if step == 0


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?

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.

yorickpeterse · 2015-03-15T12:22:46Z

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 it blocks and a few more describe blocks to clean things up, right now it's a bit messy with the amount of expectations going on at times.

fmfdias · 2015-03-15T18:28:36Z

Hi @yorickpeterse

Thanks for the review :)
I replied to some of your comments.

About the spec cleaning, I'll gladly help clearing out this specs.
My main objective was to introduce specs for the new behavior without changing too much of specs that don't need to change to fulfill the purpose. I will continue to proceed this way unless you guys ask for it, like in this case :)

Thank you!
Have a nice Sunday!

fmfdias · 2015-03-18T00:12:13Z

Hi @yorickpeterse,

Started the spec cleanup/reorganization.

Removed the specs that were testing implementations details
Reorganized the test into groups with similar properties
Tried to improve their texts (I think they may still need some improvement)
Changed from be_kind_of to be_an_instance_of

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.
Thank you!

yorickpeterse · 2015-03-19T20:04:02Z

About the ruby_bug guards, should I remove them too?

Yeah, they can be removed. I'll take a closer look at the rest this weekend.

yorickpeterse · 2015-04-12T19:51:55Z

Having taken another look, I still stand by having to somehow refactor (out) Object#get_args. I'll play around with the changes during the coming week or so to see if I can come up with a nice way to solve this problem.

fmfdias · 2015-04-29T00:20:16Z

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.
When using the shared spec, a lambda that does the transformation is defined for each of the argument type that we want to test.
Most important, to help understand what's being done I added some comments near the lambdas definitions and a comment on the shared spec definition.

While doing this, I also synced master changes into the branch.

Thanks!

yorickpeterse · 2015-04-29T08:52:13Z

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.

fmfdias · 2015-04-29T22:24:45Z

Hi @yorickpeterse,

Thanks!
I think I see your point... Since the behavior of the method is the "same" no whatever the argument style is, we may skip repeating it for different form types.
The idea behind this is that they are different arguments that have the same meaning, much like having an alias method. In those cases we also test that the alias method behaves like the original one.
To complicate things a bit, we can mix positional and keyword arguments style and this is also being tested.
An example of why this might come in handy: I ran the test against jruby 9000 pre2 and the test enters in a infinite loop in the shared section.

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!

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.

brixen · 2016-04-06T19:32:30Z

@fmfdias sorry it took so long to merge this. Thanks for the help!

fmfdias force-pushed the add_numeric_step_kw_args branch from b5fc9bc to 6031865 Compare March 12, 2015 23:18

yorickpeterse reviewed Mar 12, 2015
View reviewed changes

yorickpeterse added Needs Feedback Ruby 2.1 labels Mar 13, 2015

fmfdias force-pushed the add_numeric_step_kw_args branch from 6031865 to 7831e82 Compare March 13, 2015 00:54

yorickpeterse reviewed Mar 15, 2015
View reviewed changes

fmfdias force-pushed the add_numeric_step_kw_args branch from 7831e82 to 94ea394 Compare March 17, 2015 23:29

yorickpeterse self-assigned this Mar 19, 2015

fmfdias force-pushed the add_numeric_step_kw_args branch from 94ea394 to df28f06 Compare March 20, 2015 20:39

fmfdias force-pushed the add_numeric_step_kw_args branch from df28f06 to c8be76f Compare April 28, 2015 23:34

fmfdias force-pushed the add_numeric_step_kw_args branch from c8be76f to 415f0fe Compare April 29, 2015 22:22

fmfdias mentioned this pull request Apr 29, 2015

Numeric#step issues jruby/jruby#2892

Closed

fmfdias force-pushed the add_numeric_step_kw_args branch from 415f0fe to aa8568c Compare May 1, 2015 18:41

fmfdias force-pushed the add_numeric_step_kw_args branch 2 times, most recently from 34a4d79 to 8cbd36d Compare June 6, 2015 14:29

fmfdias added 17 commits June 13, 2015 15:18

Adds Numeric#step keyword arguments specs.

0c056f2

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.

Implements keyword arguments on Numeric#step.

bf2d9cd

Move get_args method to a helper file.

d6ad6c3

Removes unneeded parenthesis from an if.

e6f251a

Switch from be_kind_of to be_an_instance_of.

83eb732

Removes specs that only test implementation details.

7a2f858

First take at the Numeric#step spec reorganization.

0e2be45

Avoid false positive due to type conversions on Numeric types.

c2f4895

Restructure step as string specs

b212953

Adds more specs and fix a spec text.

fde6a01

Use value instead of self.

3f39c32

When one the params is a float, the results are floats.

Replace Float::INFINITY value by infinity_value helper.

469b904

Remove ruby_bug guards

51a278b

Apply another approach to share the specs for different argument types.

c9068fc

Adds an extra test to the keywords argument style.

9aec8f8

Improve shared spec comment.

02ad34c

Improve test description to avoid misinterpretation.

bd606b5

fmfdias force-pushed the add_numeric_step_kw_args branch from 8cbd36d to bd606b5 Compare June 13, 2015 14:19

brixen merged commit bd606b5 into rubinius:master Apr 6, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add numeric step kw args #3356

Add numeric step kw args #3356

fmfdias commented Mar 12, 2015

yorickpeterse commented Mar 12, 2015

fmfdias commented Mar 12, 2015

yorickpeterse commented Mar 12, 2015

yorickpeterse Mar 12, 2015

fmfdias Mar 12, 2015

fmfdias commented Mar 12, 2015

fmfdias commented Mar 13, 2015

yorickpeterse commented Mar 13, 2015

fmfdias commented Mar 13, 2015

yorickpeterse Mar 15, 2015

fmfdias Mar 15, 2015

yorickpeterse commented Mar 15, 2015

fmfdias commented Mar 15, 2015

fmfdias commented Mar 18, 2015

yorickpeterse commented Mar 19, 2015

yorickpeterse commented Apr 12, 2015

fmfdias commented Apr 29, 2015

yorickpeterse commented Apr 29, 2015

fmfdias commented Apr 29, 2015

brixen commented Apr 6, 2016


		describe :numeric_step, :shared => true do

		def get_args(*args)

Add numeric step kw args #3356

Add numeric step kw args #3356

Conversation

fmfdias commented Mar 12, 2015

yorickpeterse commented Mar 12, 2015

fmfdias commented Mar 12, 2015

yorickpeterse commented Mar 12, 2015

yorickpeterse Mar 12, 2015

Choose a reason for hiding this comment

fmfdias Mar 12, 2015

Choose a reason for hiding this comment

fmfdias commented Mar 12, 2015

fmfdias commented Mar 13, 2015

yorickpeterse commented Mar 13, 2015

fmfdias commented Mar 13, 2015

yorickpeterse Mar 15, 2015

Choose a reason for hiding this comment

fmfdias Mar 15, 2015

Choose a reason for hiding this comment

yorickpeterse commented Mar 15, 2015

fmfdias commented Mar 15, 2015

fmfdias commented Mar 18, 2015

yorickpeterse commented Mar 19, 2015

yorickpeterse commented Apr 12, 2015

fmfdias commented Apr 29, 2015

yorickpeterse commented Apr 29, 2015

fmfdias commented Apr 29, 2015

brixen commented Apr 6, 2016