Support keyword arguments when checking for valid arity.

[xaviershay]

This changes the error message that was included in the ArgumentError
raised when arity was incorrect. It used to match the default Ruby
message, it now uses our enhanced error message. While it would be
technically more correct to emulate the Ruby one, ours is more useful and the
logic required would now have to be more complicated to support keyword
arguments.

Fixes #431.

R @alindeman @myronmarston

[alindeman]

Wow, it's been a long time since I've seen this operator used.

[myronmarston]

I wonder if ArityCalculator is no longer the best name for that class? I usually think of arity as just being the count of args. Maybe MethodSignatureComparer or MethodSignatureValidator or MethodSignatureVerifier is better? "Method signature" seems like a good name for the idea of checking if some given arguments will be valid according to whatever arg info ruby provides us about a method.

[myronmarston]

I don't have any projects on ruby 2.0 yet and haven't played around with kw args so this might be totally wrong, but should we also check for invalid optional keyword args? e.g. a keyword arg that is given but that the method does not accept? Or is that not even a valid concept according to the semantics of how the ruby feature works? (You can tell I really don't know much about this language feature yet!)

[xaviershay]

oh good call (I haven't used these much either)

irb(main):001:0> def x(a:1); end
=> nil
irb(main):002:0> x(b: 2)
ArgumentError: unknown keyword: b

Will tackle in a new PR, tracking here: #545

[myronmarston]

I'm excited to get support for newer ruby features like this in rspec 3 :).

As you probably gathered from what I said above, I'm pretty clueless about the details of the kw args feature in ruby 2.0 and 2.1, having never used it or done more than a cursory reading about it when 2.0 came out. Do you know of a resource that goes into detail about all the different cases and how ruby handles them? I'd like to understand them better.

[xaviershay]

I haven't seen a good document that went beyond the trivial parts of it. Playing around with them, arity, and parameters in IRB is a good way to explore. I should probably write a blog post :P

[xaviershay]

@myronmarston I believe I have addressed all of your comments.

I settled on MethodSignature as a new name for ArityCalculator because it's a more direct name for the concept, and I really like avoiding nouned verbs when I have the chance :) With this change, accepts? felt better than matches?.

I also moved the top-level ruby features method to their own module in the library itself, which improved the MethodSignature code as well as the specs.

Don't merge this as is - when we're happy with it I want to squash down the commits.

[myronmarston]

So I just figured something out that may be useful...if we capture the args like this:

def foo(*non_kw_args, **kw_args)
end

...we explicitly get a non_kw_args array and a kw_args hash. I think most of the arity confusion centers around the fact that off of Method#parameters, kw args are listed as individual arguments (and thus, it seems like they should be individually counted in the arity), but when you capture all passed args with *args (lacking **kw_args), they get collapsed into a single hash arg (and thus, are not individually listed in the args array). This makes arity discussions super confusing as the declaration vs calling semantics don't appear to line up.

However, once we explicitly split kw args vs non-kw args, I think it becomes much easier to model a method signature and see if a given set of args is acceptable. Specifically, we can model the following things easily:

• Minimum number of non-kw args
• Maximum number of non-kw args
• Allowed kw args
• Required kw args

When we capture the args, we can compare the non-kw args against just the first two attributes and compare the kw args just against the last two attributes.

Then we don't even have to talk about arity, and you'd only need to use the arity method on 1.8.7; on 1.9+ you could do everything via parameters, which gives you much more info and is much less confusing.

[xaviershay]

I think that makes sense, particularly for when we add allowed optional args support. OK to tackle it in that PR?

[myronmarston]

I think that makes sense, particularly for when we add allowed optional args support. OK to tackle it in that PR?

If we merge this PR and release 3.0.0.beta2 w/o the changes coming in that other PR, will that cause an inconsistent/confusing experience for users? My instinct is that supporting some aspects of kw arg method signature verification (but not all aspects) is confusing, and I think I'd rather release beta2 without any kw arg support than to have it have partial kw arg support. So if you plan to keep working on this in the short term before beta2 is released you may want to keep at it on this PR; otherwise, after beat2 is released this can probably be merged as-is (sans a rebase to deal with merge conflicts and I also want to re-review now that you've addressed my feedback -- haven't looked at your changes yet).

[xaviershay]

@myronmarston I added support for allowed keywords, and refactored MethodSignature a little. I'm still not totally happy with it:

• classify is a terrible name.
• repeat of required_keyword_args - keywords_args and friends suggests introducing another class, but I tried this and it didn't feel like it made anything easier.

[myronmarston]

This doc string is confusing; you're not dealing with any required keyword args in this example.

[myronmarston]

Woops, looks like github was showing me an outdated view even though it showed me your most recent comment, so I left comments on old code. Ignore them -- I'll re-review what you have now.

[myronmarston]

I think within_range? is confusing when used with a method that has kw args. It may or may not accept 2 args depending on what they are:

irb(main):001:0> def arity_kw(x, y:1, z:2); end
=> nil
irb(main):002:0> arity_kw(nil, nil)
ArgumentError: wrong number of arguments (2 for 1)
    from (irb):1:in `arity_kw'
    from (irb):2
    from /Users/myron/.rubies/ruby-2.0.0-p247/bin/irb:12:in `<main>'
irb(main):003:0> arity_kw(nil, {})
=> nil

I think we're better of testing these methods with an actual list of args rather than using a helper where you pass a number since the values are important.

[myronmarston]

@xaviershay -- I've got some seeds of ideas for further improvement but it's the kind of thing I need to play with a bit to really flesh out. I don't think it'll change any external behavior but it should make it easier to maintain and reason about. So maybe we should merge this and then I can do a PR on top of this?

[myronmarston]

BTW, this addresses #545 now, right?

[xaviershay]

After sleeping on it I figured out how to do this, will submit further improvement today.

Yes, it addresses #545.

[xaviershay]

@myronmarston latest commit isn't a great diff, but I think is a better way of doing this. I had to noun a verb, but everything else became clearer.

[myronmarston]

I had to noun a verb

I love the metaness of this, since it uses "noun" as a verb and "verb" as a noun :).

[myronmarston]

This looks great, @xaviershay. Merging.

[xaviershay]

Dammit you didn't let me rebase+squash :(

[myronmarston]

Dammit you didn't let me rebase+squash :(

Didn't realize you wanted to, sorry.

[xaviershay]

s'ok we're still friends