Properly support and deprecate default hash values provided as second positional argument to setting for Ruby 2.6 and 2.7

[timriley]

Before this change, if we defined a setting like this:

setting :default_options, {foo: "bar"}

It would fail on Ruby 2.6 and 2.7 because inside DSL#setting, whose signature is this:

def setting(name, default = Undefined, **options, &block)

The default value remains as Undefined, and options becomes {foo: "bar"}, which then sees an exception raised inside #ensure_valid_options because (of course) they are not valid options for setting.

What we really want is for that hash provided as the second positional parameter to be assigned to default, and options to remain as {}.

---

Per the extended code comments included in the change, we take the following approach to handle this case:

In Ruby 2.6 and 2.7, when a hash is given as the second positional argument (i.e. the hash is intended to be the setting's default value), and there are no other keyword arguments given, the hash is assigned to the options variable instead of default.

For example, for this setting:

setting :hash_setting, {my_hash: true}

We'll have a default of Undefined and an options of {my_hash: true}

If any additional keyword arguments are provided, e.g.:

setting :hash_setting, {my_hash: true}, reader: true

Then we'll have a default of {my_hash: true} and an options of {reader: true}, which is what we want.

To work around that first case and ensure our (deprecated) backwards compatibility holds for Ruby 2.6 and 2.7, we extract all invalid options from options, and if there are no remaining valid options (i.e. if there were no keyword arguments given), then we can infer the invalid options to be a default hash value for the setting.

This approach also preserves the behavior of raising an ArgumentError when a distinct hash is not intentionally provided as the second positional argument (i.e. it's not enclosed in braces), and instead invalid keyword arguments are given alongside valid ones. So this setting:

setting :some_setting, invalid_option: true, reader: true

Would raise an ArgumentError as expected.

However, the one case we can't catch here is when invalid options are supplied without hash literal braces, but there are no other keyword arguments supplied. In this case, a setting like:

setting :hash_setting, my_hash: true

Is parsed identically to the first case described above:

setting :hash_setting, {my_hash: true}

So in both of these cases, the default value will become {my_hash: true}. We consider this unlikely to be a problem in practice, since users are not likely to be providing invalid options to setting and expecting them to be ignored. Additionally, the deprecation messages will make the new behavior obvious, and encourage the users to upgrade their setting definitions.

[timriley]

@flash-gordon @solnic are you aware of any trickery that might let us get these arguments into the right local variables?

[timriley]

A possible hack (but potentially good enough given this is a backwards compatibility shim and it's only problematic on older Rubies?): if default is Undefined, and options is a non-empty hash, then take all the valid keys out of options, and if there is still a non-empty hash remaining, treat that as the default. We could also add some extra deprecation warnings for the special case, too, e.g. "We're inferring a default value from your provided hash, please provide it as default: instead blah blah".

One thing I should note here is that this is only a problem if there are no other options provided, so this case below is actually fine:

setting :foo, {foo: "bar"}, reader: true

In this case, the default is {foo: "bar"} and options is reader: true.

[flash-gordon]

A possible hack (but potentially good enough given this is a backwards compatibility shim and it's only problematic on older Rubies?): if default is Undefined, and options is a non-empty hash, then take all the valid keys out of options, and if there is still a non-empty hash remaining, treat that as the default. We could also add some extra deprecation warnings for the special case, too, e.g. "We're inferring a default value from your provided hash, please provide it as default: instead blah blah".

I'd go with this. I don't think bothering with warnings is worth it though

[timriley]

@flash-gordon Thanks for the feedback :) Yeah, it felt reasonable to me too when I thought of it — I'll go ahead and prepare this, then!

[AMHOL]

Had this problem a while back #14

[timriley]

@solnic @flash-gordon I've addressed this issue now and updated the PR description to describe my approach.

Would you mind checking this out and letting me know if you're happy with it?

Once we merge this in, then it means the change for the default value from the second positional argument to the default: keyword argument is as backwards compatible as can possibly be, which should give us confidence to make this next dry-configurable release without having to employ any additional compatibility shims in the dry-rb gems that are already using it (i.e. following the approach I described in #120 (comment)).

I'll still do another couple of full rounds of testing inside the gems listed in #120, of course, but I hope that should be quite straightforward after this adjustment here.

[waiting-for-dev]

Thanks, @timriley, amazing work!

I agree with the solution. When we have both an unknown key and a known key, we have to favor one of the two possible options:

1 - The user wants to use a known key on its default hash.
2 - The user misspelled a known key on its kwargs.

At this point, known keys are:

dry-configurable/lib/dry/configurable/setting.rb

Line 18 in 793a203

OPTIONS = %i[input default reader constructor cloneable settings].freeze

So, probably, there're no big chances of name collision.

With your solution, we're going with the second option, which is better because users will get an error because of unknown keyword arguments, which will help them work around it.

Other than that, I think you'll need to remove those FileUtils.pwd before merging. Please, see #119 .

[flash-gordon]

LGTM 👍

[flash-gordon]

options.partition { |k, _| Setting::OPTIONS.include?(k) }.map(&:to_h)

[timriley]

So much neater, thanks @flash-gordon! I've applied this now.

[timriley]

Thanks for the feedback, folks! Looks like we're all in agreement on approach here. I'm going to merge this now so I can get on with the testing.