Skip to content
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 attribute readers for unknown attributes #89

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jonspalmer
Copy link

default behavior remains unchanged

Unknown params and options are ignored but stored in their own attr_reader

class Test::Foo
 extend Dry::Initializer

 param :foo
 param :bar, optional: true
end

foo = Test::Foo.new(1, 2, 3, 4)
foo.__dry_initializer_unknown_params__ # => [3, 4]
class Test::Foo
 extend Dry::Initializer

 option :foo
 options :bar, optional: true
end

foo = Test::Foo.new(foo: 1, bar: 2, fizz: 3, buzz: 4)
foo.__dry_initializer_unknown_options__ # => {fizz: 3, buzz: 4}

rest params and rest options names are configurable

class Test::Foo
 extend Dry::Initializer

 param :foo
 param :bar, optional: true
 rest_param :unknown_params
end

foo = Test::Foo.new(1, 2, 3, 4)
foo.unknown_params # => [3, 4]
class Test::Foo
 extend Dry::Initializer

 option :foo
 options :bar, optional: true
 rest_options :unknown_options
end

foo = Test::Foo.new(foo: 1, bar: 2, fizz: 3, buzz: 4)
foo.unknown_options # => {fizz: 3, buzz: 4}

rest params and rest options can be turned off for strict mode

This adds support for strict checking of params and options: #68

class Test::Foo
 extend Dry::Initializer

 param :foo
 param :bar, optional: true
 rest_param false
end

Test::Foo.new(1, 2, 3, 4) => raises ArgumentError
class Test::Foo
 extend Dry::Initializer

 option :foo
 options :bar, optional: true
 rest_options :unknown_options
end

Test::Foo.new(foo: 1, bar: 2, fizz: 3, buzz: 4) # raises ArgumentError

 * default behavior remains unchanged. Unknown params and options
   are ignored but stored in their own attr_reader
 * rest params and rest options names are configurable
 * rest params and rest options can be turned off for strict mode
@jonspalmer jonspalmer requested a review from solnic as a code owner July 17, 2021 21:22
option :bar, proc(&:to_s), optional: true
option :"some foo", as: :bar, optional: true
option :bar, proc(&:to_s), optional: true
option :some_foo, as: :bar, optional: true
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

String keyword argument names aren't allowed in 2.6. This is perhaps a very small breaking change but frankly its unlikely anyone on 2.6 was taking advantage of it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think here is a potential for the backward incompatibility. This test is about not string, but symbol keys. What is essential is that the keys don't need to be a valid ruby methods.

For example, we should can handle this assignment:

class Foo
  extend Dry::Initializer
  
  option :"!!! my weird key", as: :weird_key
end

foo = Foo.new("!!! my weird key": 1) # notice the key is a symbol, not a string
foo.weird_key # => 1

The goal of this feature to make it possible handling any keys in the income hash (maybe with a forced symbolization like below)

data = { 1 => 2 }.transform_keys { |k| k.to_s.to_sym }

@jonspalmer
Copy link
Author

Rubocop linting is needed but I'm loath to do that first as the class/module nesting changes lead to every line of the files getting whitespace changes that would obscure the code really being touched.

@jonspalmer
Copy link
Author

cc @nepalez perhaps for a reivew?

Copy link
Member

@nepalez nepalez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the feature and it looks to me that the implementation is good enough (I mean it shouldn't cause an essential loss in performance). But I'd like to be sure that it is backward compatible to processing all the keys which fall short of the requirements to the Ruby method names.

option :bar, proc(&:to_s), optional: true
option :"some foo", as: :bar, optional: true
option :bar, proc(&:to_s), optional: true
option :some_foo, as: :bar, optional: true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think here is a potential for the backward incompatibility. This test is about not string, but symbol keys. What is essential is that the keys don't need to be a valid ruby methods.

For example, we should can handle this assignment:

class Foo
  extend Dry::Initializer
  
  option :"!!! my weird key", as: :weird_key
end

foo = Foo.new("!!! my weird key": 1) # notice the key is a symbol, not a string
foo.weird_key # => 1

The goal of this feature to make it possible handling any keys in the income hash (maybe with a forced symbolization like below)

data = { 1 => 2 }.transform_keys { |k| k.to_s.to_sym }

@@ -27,7 +29,7 @@ class Test::Foo
end

context "when renamed" do
subject { Test::Foo.new "some foo": :BAZ }
subject { Test::Foo.new some_foo: :BAZ }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And the same problem is here

Copy link
Author

@jonspalmer jonspalmer Jul 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why I originally made this change. Perhaps some intermediate state. I've reverted its and specs still pass. Can you approve the actions here so we can get spec output?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, I meant a different change. The original version got a space inside "some foo" to empasize this is not a proper Ruby method name. In your version you just switched from symbol :some_foo to the string "some_foo", but both satisfy the metnod name requirements.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I missed that. The challenge with supporting that is we can't have options like that explicitly defined in the signature. We'd have to bury that logic in unpacking the ** arguments. Honestly I find this feature pretty surprising - I'm unclear on the real world use cases for it. Aliasing just in the constructor call when all other interactions are through the legal ruby method name passed to as is a little peculiar. Can you explain more?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The particular challenge being Introspecting the dry initializer doesn't accurately return the constructor arguments my_foo.method(:__dry_initializer_initialize__).parameters

@simonc
Copy link

simonc commented Aug 17, 2022

Hi there 😊 Was this idea abandoned? It would be a really great addition!

@hangsu
Copy link

hangsu commented Nov 20, 2022

I'd also like to see this idea revived. Is there any way I can help?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants