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

feature: allow to pass raw hash as defaults settings #10

Merged
merged 1 commit into from
Dec 20, 2017

Conversation

dsalahutdinov
Copy link
Contributor

Hi!

Want to support somethings like this to allow to pass options, which will be applied after defaults for new instance of config
Usage looks like this:

Sniffer::Config.new(
  enabled: true,
  storage: {capacity: 500}
)

Will be used here in aderyabin/sniffer#35

Gemfile Outdated
@@ -14,3 +14,5 @@ if File.exist?(local_gemfile)
else
gem 'rails', '~> 5.0'
end

gem 'rubocop', git: '[email protected]:bbatsov/rubocop.git'
Copy link
Owner

Choose a reason for hiding this comment

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

Don't think we need it here.

For local development you can use Gemfile.local.

@@ -24,6 +23,6 @@ Gem::Specification.new do |s|
s.required_ruby_version = '>= 2.2'

s.add_development_dependency "rspec", "~> 3.5"
s.add_development_dependency "rubocop", "~> 0.49"
s.add_development_dependency "rubocop", "~> 0.50"
Copy link
Owner

Choose a reason for hiding this comment

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

Let's upgrade to 0.52

end
end

attr_reader :config_name

def initialize(config_name = nil, do_load = true)
def initialize(config_name: nil, do_load: true, config: {})
Copy link
Owner

Choose a reason for hiding this comment

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

Let's use the following signature initialize(explicit_values = {}, name: nil, load: true).

Don't we want to have explicitly provided values non-overridable by other sources (file, env)?

I mean, if I do something like config = Config.new({a: 1}) I want to make sure that the value of config.a doesn't change. Am I right?

Then we have to apply these explicit_values after everything is loaded.

Also, we should implement the same behaviour for #reload method (and we can re-use it in #initialize):

def initialize(explicit_values = {}, name: nil, reload: true)
  @config_name = config_name || self.class.config_name
  raise ArgumentError, "Config name is missing" unless @config_name
  reload(explicit_values) if reload
end

def reload(explicit_values = {})
  clear
  load(explicit_values)
  self
end

def load(explicit_values = nil)
  config = load_from_sources((self.class.defaults || {}).deep_dup)
  config.merge!(explicit_values) unless explicit_values.nil?
  config.each do #...
end

Copy link
Contributor Author

@dsalahutdinov dsalahutdinov Dec 19, 2017

Choose a reason for hiding this comment

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

Hi, Vlidimir!
Thanks for review!

this kind of signature is not good for the case, because of it makes you to pass all the arguments if you want ruby to recognize it properly:

def test(a = {}, b: 'b', c: 'c')
  puts a.inspect
  puts b
  puts c
end

test()
test({sample: 'string'}) #expect sample be the key ArgumentError: unknown keyword: sample like the next
ArgumentError: unknown keyword: sample
	from (irb):1:in `test'
	from (irb):7
	from /usr/local/rvm/rubies/ruby-2.2.7/bin/irb:11:in `<main>'

test(sample: 'string')
test({sample: 'string'}, b: 1, c: 2) #works as expected, but needs to pass other args to make ruby recognize first hash as 'a' param

So I suppose to make signature like this:

def initialize(explicit_values: {}, name: nil, load: true)
end

to allow to pass only needed arguments

Copy link
Contributor Author

@dsalahutdinov dsalahutdinov Dec 19, 2017

Choose a reason for hiding this comment

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

So in those case i would also like to change the signatures of methods load and reload to look similar:

def load(explicit_values: {})
end

def reload(explicit_values: {})
end

@@ -43,22 +43,22 @@ def config_name(val = nil)
#
# my_config = Anyway::Config.for(:my_app)
# # will load data from config/my_app.yml, secrets.my_app, ENV["MY_APP_*"]
def for(name)
new(name, false).load_from_sources
def for(explicit_values: {}, name: nil)
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think we need explicit values here, 'cause this method returns a plain Hash

Let's leave as is.

end
end

attr_reader :config_name

def initialize(config_name = nil, do_load = true)
@config_name = config_name || self.class.config_name
def initialize(explicit_values: {}, name: nil, load: true)
Copy link
Owner

Choose a reason for hiding this comment

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

Looks that I've proposed a wrong solution: there gonna be a breaking change.

Let's do the following:

def initialize(config_name = nil, do_load = nil, name: nil, load: true, overrides: {})
  unless config_name.nil? && do_load.nil?
    warn "[Deprecated] Positional arguments for Anyway::Config#initialize will be removed in 1.2.0. Use keyword arguments instead: initialize(name:, load:, overrides:)"
  end

  name = config_name unless config_name.nil?
  load = do_load unless do_load.nil?

  ...
end

And let's rename explicit_values to overrides.

raise ArgumentError, "Config name is missing" unless @config_name
load if do_load
load(explicit_values: explicit_values) if load
Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't there be self.load?

@dsalahutdinov
Copy link
Contributor Author

It's done

@palkan palkan merged commit 52dbf69 into palkan:master Dec 20, 2017
@palkan
Copy link
Owner

palkan commented Dec 20, 2017

Thanks!

Released as 1.1.3

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.

2 participants