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

Merged
merged 17 commits into from Apr 6, 2016

Conversation

Projects
None yet
3 participants
@fmfdias
Contributor

fmfdias commented Mar 12, 2015

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

This comment has been minimized.

Member

YorickPeterse commented Mar 12, 2015

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

@fmfdias fmfdias force-pushed the fmfdias:add_numeric_step_kw_args branch from b5fc9bc to 6031865 Mar 12, 2015

@fmfdias

This comment has been minimized.

Contributor

fmfdias commented Mar 12, 2015

Rebase done!

@YorickPeterse

This comment has been minimized.

Member

YorickPeterse commented Mar 12, 2015

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)

This comment has been minimized.

@YorickPeterse

YorickPeterse Mar 12, 2015

Member

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.

This comment has been minimized.

@fmfdias

fmfdias Mar 12, 2015

Contributor

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

This comment has been minimized.

Contributor

fmfdias commented Mar 12, 2015

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 fmfdias force-pushed the fmfdias:add_numeric_step_kw_args branch from 6031865 to 7831e82 Mar 13, 2015

@fmfdias

This comment has been minimized.

Contributor

fmfdias commented Mar 13, 2015

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

This comment has been minimized.

Member

YorickPeterse commented Mar 13, 2015

@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

This comment has been minimized.

Contributor

fmfdias commented Mar 13, 2015

Ok :) Let me know of any changes needed!

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

This comment has been minimized.

@YorickPeterse

YorickPeterse Mar 15, 2015

Member

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?

This comment has been minimized.

@fmfdias

fmfdias Mar 15, 2015

Contributor

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.

value = values[0]
limit = values[1]
step = values[2]
asc = values[3]
is_float = values[4]
if(step == 0)
while true
yield self

This comment has been minimized.

@YorickPeterse

YorickPeterse Mar 15, 2015

Member

How is this supposed to ever return from the method (other than the supplied block calling break)?

This comment has been minimized.

@fmfdias

fmfdias Mar 15, 2015

Contributor

At far as I could see, it's not supposed too. It's how MRI behaves... If step, given by the by: keyword argument, is 0, you get an infinite loop.
From the Numeric#step documentation:

The loop finishes when the value to be passed to the block is greater than limit (if step is positive) or less than limit (if step is negative), where limit is defaulted to infinity.

You can still do stuff like

10.step(to: 15, by: 0).take(5)
=> [10, 10, 10, 10, 10]

... although I don't immediately see it's usefulness.

value = values[0]
limit = values[1]
step = values[2]
asc = values[3]
is_float = values[4]
if(step == 0)

This comment has been minimized.

@YorickPeterse

YorickPeterse Mar 15, 2015

Member

Use if step == 0, the parenthesis are not required here.

This comment has been minimized.

@fmfdias

fmfdias Mar 15, 2015

Contributor

👍

@@ -0,0 +1,13 @@
class Object
def get_args(args_type, *args)

This comment has been minimized.

@YorickPeterse

YorickPeterse Mar 15, 2015

Member

Thinking of it, the specs should probably be refactored to remove the need for this helper method. Right now it's hard to see what exactly the specs are doing as they rely on this rather odd helper method. For example, the behaviour of this method is quite different based on the arguments and/or argument size given to it.

This comment has been minimized.

@fmfdias

fmfdias Mar 15, 2015

Contributor

Besides the step = 0 vs by: 0 difference and the ArgumentError raising when repeating arguments when using positional and keyword arguments, the results should always be the same.

The idea behind this was to avoid duplicating the specs 3 times where the only thing that changes is how you pass the arguments.

I can thing on other options, and I'm open to suggestions, being it a different approach or clarifying this method to avoid confusion, but I think that duplicating the tests is the worst one.

1.send(@method, *get_args(@args_type, 0, 2)).should be_an_instance_of(enumerator_class)
end
it "returns an Enumerator when not passed a block and self < stop" do

This comment has been minimized.

@YorickPeterse

YorickPeterse Mar 15, 2015

Member

This and the above spec should probably be wrapped in another describe like so as this makes them a bit easier to grok (at least in my opinion):

describe 'with a block' do
  it 'returns an Enumerator when step is 0' do

  end

  ...
end
0.send(@method, *get_args(@args_type, 5, 2)).to_a.should == [0, 2, 4]
end
describe "with [stop, step]" do

This comment has been minimized.

@YorickPeterse

YorickPeterse Mar 15, 2015

Member

Use regular English here, so with stop and step or something along those lines.

@obj.send(@method, *get_args(@args_type, @stop, @step), &@prc)
ScratchPad.recorded.should == [@obj, @obj, @obj]

This comment has been minimized.

@YorickPeterse

YorickPeterse Mar 15, 2015

Member

These tests don't test behaviour, they test implementation details. Specs shouldn't dictate what methods internally are used except for a few rare edge cases where calling a certain method is part of the defined behaviour. In this case it's crazy to test that the objects called > and +. Same applies to the tests below.

end
end
describe "Numeric#step with [stop, step] when self and stop are Fixnums but step is a String" do

This comment has been minimized.

@YorickPeterse

YorickPeterse Mar 15, 2015

Member

This describe block, along with the one above, should be broken up into separate blocks. For example:

describe 'Numeric#step' do
  describe 'with stop, step and self as a Fixnum' do
    it 'yields only Fixnums' do
      ...
    end
  end

  describe 'with stop and self as a Fixnum and step as a String' do
    it 'returns an Enumerator if not given a block' do
      ...
    end
  end
end

While this introduces some extra indentation levels it cuts down the length of each individual spec/describe description. It also groups together related specs instead of them being toplevel it blocks.

end
it "returns an Enumerator when step is 0" do
1.send(@method, *get_args(@args_type, 2, 0)).should be_an_instance_of(enumerator_class)

This comment has been minimized.

@YorickPeterse

YorickPeterse Mar 15, 2015

Member

Where does enumerator_class come from?

This comment has been minimized.

@fmfdias

fmfdias Mar 15, 2015

Contributor

It's being used all over the place :)
I think it comes from here.

lambda { 1.send(@method, *get_args(@args_type, 5, "1")) {} }.should raise_error(ArgumentError)
lambda { 1.send(@method, *get_args(@args_type, 5, "0.1")) {} }.should raise_error(ArgumentError)
lambda { 1.send(@method, *get_args(@args_type, 5, "1/3")) {} }.should raise_error(ArgumentError)
lambda { 1.send(@method, *get_args(@args_type, 5, "foo")) {} }.should raise_error(ArgumentError)

This comment has been minimized.

@YorickPeterse

YorickPeterse Mar 15, 2015

Member

All these expectations should be moved into separate it blocks, same goes for the specs below. For example:

describe 'with a block' do
  it 'raises ArgumentError with step "0.1"' do

  end

  it 'raises ArgumentError with step "1/3"' do

  end

  ...
end
describe "Numeric#step with [stop, step]" do
it "yields only Floats when self is a Float" do
1.5.send(@method, *get_args(@args_type, 5, 1)) { |x| x.should be_kind_of(Float) }

This comment has been minimized.

@YorickPeterse

YorickPeterse Mar 15, 2015

Member

Use should be_an_instance_of instead as we want to test if the object is an actual instance.

@YorickPeterse

This comment has been minimized.

Member

YorickPeterse commented Mar 15, 2015

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

This comment has been minimized.

Contributor

fmfdias commented Mar 15, 2015

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 fmfdias force-pushed the fmfdias:add_numeric_step_kw_args branch from 7831e82 to 94ea394 Mar 17, 2015

@fmfdias

This comment has been minimized.

Contributor

fmfdias commented Mar 18, 2015

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

This comment has been minimized.

Member

YorickPeterse commented Mar 19, 2015

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 YorickPeterse self-assigned this Mar 19, 2015

@fmfdias fmfdias force-pushed the fmfdias:add_numeric_step_kw_args branch from 94ea394 to df28f06 Mar 20, 2015

@YorickPeterse

This comment has been minimized.

Member

YorickPeterse commented Apr 12, 2015

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 fmfdias force-pushed the fmfdias:add_numeric_step_kw_args branch from df28f06 to c8be76f Apr 28, 2015

@fmfdias

This comment has been minimized.

Contributor

fmfdias commented Apr 29, 2015

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

This comment has been minimized.

Member

YorickPeterse commented Apr 29, 2015

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 fmfdias force-pushed the fmfdias:add_numeric_step_kw_args branch from c8be76f to 415f0fe Apr 29, 2015

@fmfdias

This comment has been minimized.

Contributor

fmfdias commented Apr 29, 2015

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!

@fmfdias fmfdias referenced this pull request Apr 29, 2015

Open

Numeric#step issues #2892

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

@fmfdias fmfdias force-pushed the fmfdias:add_numeric_step_kw_args branch 2 times, most recently from 34a4d79 to 8cbd36d May 30, 2015

@fmfdias fmfdias force-pushed the fmfdias:add_numeric_step_kw_args branch from 8cbd36d to bd606b5 Jun 13, 2015

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@brixen

This comment has been minimized.

Member

brixen commented Apr 6, 2016

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment