Improve method signature / argument validation #548
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It's a bit shorter but still obvious what it means.
- The signature of a method is a concept in its own right and deserves to be represented as its own object. - This gives a clear delineaziation of responsibilities between extracting method signature info and verifying arguments against that info. - The new `classify_parameters` approach should perform a bit better than what we had before; it iterates over `method.parameters` once total where as before it would iterate over it 3 times.
A last-arg hash should only be treated as kw args if the method has kw arg params.
- It was odd to have it memoize internally but accept an arg. - Simpler just to call it once in `initialize`.
I find this:
foo(nil, :a => 1)
...to be much less noisy than this:
foo([nil, { :a => 1 }])
The first form more closely matches how people pass args
and kw args to methods, anyway.
To support `**kw_args`, `allowed_kw_args` will have to act like an infinite set. Rather than making the verifier (and any future clients) responsible for dealing with this special case, it's simpler to move the invalid/missing kw arg calculations into `MethodSignature`, where it can handle that special case internally.
BasicObject, for example, does not respond to it. It's safer to use `SomeClass === obj` than `obj.is_a?(SomeClass)`.
Mutating could lead to surprises for the caller.
It's not a ruby error object, it's just the message, so the new name is more accurate.
Arity is just the number of arguments but we check more than that now, so it's good to reflect that.
|
so beautiful |
It's a description of the arity so let's call it that.
This clears the way to use a slightly different signature implementation (e.g. for blocks).
I'm about to introduce a new `description` method and having docstrings say "it is described precisely" when not referring to the description is confusing.
|
OK, I think this is ready to merge as far as I know. Any concerns, @xaviershay? |
|
LGTM, ship it. |
myronmarston
added a commit
that referenced
this pull request
Jan 31, 2014
Improve method signature / argument validation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is some further changes on top of #543.
I've done a bit of refactoring, renamed some things to reflect new realities (e.g. it's not just an arity check anymore), and fixed a couple of bugs I noticed when playing around with kw args.
Things I still want to consider doing:
I'd still like to consider raising an error message with all of the problems rather than just the first discovered problem. I think this is simpler with the current code structure than when we discussed it before. Nevermind, I've decided I'm ok with how it is.arity. The current message may be confusing and we should look more into it.@xaviershay, the commits are pretty self-contained and since there are a bunch of changes made for different reasons (generally explained in the commit message), you may want to review this commit-by-commit.