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

Allow Hash as a possible config value #131

Merged
merged 1 commit into from
Apr 21, 2022
Merged

Conversation

ojab
Copy link
Contributor

@ojab ojab commented Mar 31, 2022

It's handy to do something like

v = {key: '"-----BEGIN RSA PRIVATE KEY-----…', key_password: 'qwerty'}
# and then
setting :openssl_key, constructor: ->(v) {
  OpenSSL::PKey.read(*v.values_at(:key, :key_password))
}

to have decrypted key in the settings instead of two strings, but dry-c complains that nil doesn't have #update. I suppose there is an expectation that if we're assigning a Hash — value is defined by setting, we shouldn't expect that.

@ojab ojab marked this pull request as ready for review March 31, 2022 09:11
@ojab ojab requested a review from solnic as a code owner March 31, 2022 09:11
@solnic
Copy link
Member

solnic commented Mar 31, 2022

I'm not sure what the actual problem is. Could you rephrase the description somehow? ie why is it trying to call update on nil?

@ojab
Copy link
Contributor Author

ojab commented Mar 31, 2022

require 'dry-configurable'

class Xxx
  extend Dry::Configurable

  setting :xxx
end

Xxx.configure do |config|
  config.update(xxx: {})
end

leads to

…/dry-configurable-0.14.0/lib/dry/configurable/config.rb:62:in `block in update': undefined method `update' for nil:NilClass (NoMethodError)

            self[key].update(value)
                     ^^^^^^^
	from …/dry-configurable-0.14.0/lib/dry/configurable/config.rb:59:in `each'

because it implies that setting is a Config if input value is a Hash and we're passing the hash, even if :xxx is not a subconfig, but a plain value.

@ojab ojab force-pushed the allow_assign_hash branch from 346d744 to 32fb48e Compare March 31, 2022 10:39
@ojab
Copy link
Contributor Author

ojab commented Mar 31, 2022

Also:

require 'dry-configurable'

class Xxx
  extend Dry::Configurable

  setting :xxx, default: {x: 1}
end


Xxx.configure do |config|
  config.update(xxx: {y: 2})
end

p Xxx.config.to_h # => {:xxx=>{:x=>1, :y=>2}}

@ojab
Copy link
Contributor Author

ojab commented Mar 31, 2022

OTOH now we can overwrite Config with a plain value :/ I'll fix it a bit later today.

@ojab
Copy link
Contributor Author

ojab commented Mar 31, 2022

BTW, shouldn't .is_a?(Hash) be .respond_to(:to_hash) instead? It's a convention that #to_hash is explicit permission for an object to be represented as a Hash.

irb(main):001:0> x = Object.new
=> #<Object:0x00007f250e9bf278>
irb(main):002:0> def x.to_hash; {x: 1, y: 2};end 
=> :to_hash
irb(main):003:0> {**x}
=> {:x=>1, :y=>2}

@ojab ojab force-pushed the allow_assign_hash branch from 32fb48e to 55407b4 Compare March 31, 2022 11:05
@ojab
Copy link
Contributor Author

ojab commented Mar 31, 2022

Added ArgumentError on assigning non-Hash to the setting.

@ojab
Copy link
Contributor Author

ojab commented Apr 20, 2022

😿

@timriley
Copy link
Member

This all makes sense and looks good to me! I'll update the is_a?(Hash) check to use to_hash in a follow-up PR, then will cut a release.

@timriley
Copy link
Member

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.

3 participants