Raise on non-existent kwargs#586
Conversation
| ALLOWED_KWARGS = %i[ | ||
| desc | ||
| default | ||
| index_errors | ||
| strip | ||
| format | ||
| digits | ||
| base | ||
| from | ||
| methods | ||
| class | ||
| converter | ||
| ].freeze |
There was a problem hiding this comment.
These were all the options I could find from the those mentioned in the docs.
Not all of these options are possible for every kind of filter; a more sophisticated version of this would verify that the given option was possible for the filter, but I wanted to stand up a PR to see what people think first.
There was a problem hiding this comment.
I do think we should have these be per filter. If we're going to check we should be as accurate as we can be.
| def add_filter(klass, name, options, &block) | ||
| raise InvalidFilterError, %("#{name}" is a reserved name) if Inputs.reserved?(name) | ||
| if (invalid_options = options.keys - ALLOWED_KWARGS).any? | ||
| raise InvalidFilterError, "invalid options: #{invalid_options.join(', ')}" |
There was a problem hiding this comment.
Is the right exception to use? Also I wanted to make sure the options were in the message to aid in debugging but not sure if the format should change.
There was a problem hiding this comment.
Ruby throws an ArgumentError for methods with invalid keyword arguments:
* def foo(bar:)
* puts bar
> end
=> :foo
> foo(bar: 'hi', baz: 'yo')
(irb):10:in 'foo': unknown keyword: :baz (ArgumentError)
from (irb):13:in '<main>'
...I think replicating this makes the most sense.
| # rubocop:disable Style/MissingRespondToMissing | ||
| def method_missing(*args, &block) | ||
| super(*args) do |klass, names, options| | ||
| super do |klass, names, options| |
There was a problem hiding this comment.
I ran rake and rubocop complained about this one, although it seems unrelated to any of my changes, not sure why it wouldn't have come up before 🤔
| string :attribute, defualt: nil | ||
| array :array, defualt: nil | ||
| string :attribute, default: nil | ||
| array :array, default: nil |
There was a problem hiding this comment.
I wasn't sure if this misspelling was intentional since the spec is about errors. But it still passes after fixing it (and my change would break it). @AaronLasseigne you would know better than I would.
There was a problem hiding this comment.
Not intentional 😬, maybe this PR is a good idea!
| # @param options [Hash] | ||
| def add_filter(klass, name, options, &block) | ||
| raise InvalidFilterError, %("#{name}" is a reserved name) if Inputs.reserved?(name) | ||
| if (invalid_options = options.keys - ALLOWED_KWARGS).any? |
There was a problem hiding this comment.
Comment on the implementation specifically. Using constant ALLOWED_KWARGS makes it impossible to extend functionality with new behavior. Consider using methods, as they can be extended.
| def add_filter(klass, name, options, &block) | ||
| raise InvalidFilterError, %("#{name}" is a reserved name) if Inputs.reserved?(name) | ||
| if (invalid_options = options.keys - ALLOWED_KWARGS).any? | ||
| raise InvalidFilterError, "invalid options: #{invalid_options.join(', ')}" |
There was a problem hiding this comment.
Filter option validation is more suitable for Filter classes as each filter can validate its own options. Consider moving them there.
|
Sorry I've been slow to get to this. Life and all. I like the idea but I have a few comments that I'll note. |
smcabrera
force-pushed
the
main
branch
2 times, most recently
from
72aa881 to
10245e7
Compare
Currently filters will allow you to include keyword arguments that don't exist, leading to potentially confusing outcomes for users. This change makes it so that unsupported keyword arguments will raise an exception, while still allowing users to extend the available options if needed.
| # Returns the list of allowed options for this filter type. | ||
| # | ||
| # @return [Array<Symbol>] | ||
| def allowed_options |
There was a problem hiding this comment.
I didn't replicate this comment on all the subclass methods, but I could if we think that's valuable
|
Trying to see if I can close up some open PRs before the new year 😅 I made a few changes in response to feedback
Also added something to the changelog, @AaronLasseigne may have thoughts on what's the best way to summarize this change as I don't think I've committed to this project before |
|
@AaronLasseigne @antulik I think I've addressed your feedback, see comment above |
3 participants
closes #585
Long-time listener first-time caller! We use active_interaction at work and it's been a big help for us using command objects in a standardized way, kudos.
See the issue for a description of what I'm trying to do here. I'm new so let me know if there are changes I can make to match the project better.