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

Separate setting definitions from config values #138

Merged
merged 4 commits into from
Sep 19, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 11 additions & 8 deletions lib/dry/configurable/class_methods.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,16 @@ def inherited(subclass)
super

subclass.instance_variable_set(:@__config_extension__, __config_extension__)
subclass.instance_variable_set(:@_settings, _settings.dup)
subclass.instance_variable_set(:@_config, config.dup) if respond_to?(:config)

new_settings = _settings.dup
subclass.instance_variable_set(:@_settings, new_settings)

# Only classes **extending** Dry::Configurable have class-level config. When
# Dry::Configurable is **included**, the class-level config method is undefined because it
# resides at the instance-level instead (see `Configurable.included`).
Comment on lines +19 to +21
Copy link
Member Author

Choose a reason for hiding this comment

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

I left this note here because I remember being confused in the past about why we may not respond to config despite it being defined right here in this module. Hopefully this helps alleviate any confusion from readers in the future.

if respond_to?(:config)
subclass.instance_variable_set(:@config, config.dup_for_settings(new_settings))
Copy link
Member Author

@timriley timriley Sep 14, 2022

Choose a reason for hiding this comment

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

Interestingly, until now we had been setting :@_config, when the actual memoized instance variable (used further below in the #config method) is @config. Setting this to the memoized ivar is in fact the desired behaviour here, so I think before now things must have been working accidentally.

Copy link
Member

Choose a reason for hiding this comment

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

@timriley it'd be good to decide on just one approach for naming private ivars, in hanami I noticed we have _foo, in dry-rb we have __foo__. I'm just mentioning it here for future consideration.

end
end

# Add a setting to the configuration
Expand Down Expand Up @@ -48,7 +56,7 @@ def setting(*args, **options, &block)
#
# @api public
def settings
@settings ||= Set[*_settings.map(&:name)]
Set[*_settings.map(&:name)]
Comment on lines -51 to +59
Copy link
Member Author

Choose a reason for hiding this comment

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

We don't want to memoize this because it'll capture stale data if called before all settings are defined.

Copy link
Member

Choose a reason for hiding this comment

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

@timriley I would remove this, we have _settings and this one should be exposed as settings IMO. This Set is mostly here for historical reasons, it was just always like that but I'm not sure if this is a valuable thing to keep.

Copy link
Member Author

@timriley timriley Sep 14, 2022

Choose a reason for hiding this comment

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

@solnic Oh great, I'm glad you think this! The settings/_settings disparity has always bothered me (especially since the more useful of the two objects is the one with the ugly underscore prefix).

I'll be happy to update it, but I'll do it in a separate PR (once this is merged) for better visibility.

Copy link
Member

Choose a reason for hiding this comment

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

@timriley the only reason why I introduced _settings was because I didn't want to break backward compatibility by changing settings from a set of symbol names to an array of setting objects.

end

# Return declared settings
Expand All @@ -66,11 +74,6 @@ def _settings
#
# @api public
def config
# The _settings provided to the Config remain shared between the class and the
# Config. This allows settings defined _after_ accessing the config to become
# available in subsequent accesses to the config. The config is duped when
# subclassing to ensure it remains distinct between subclasses and parent classes
# (see `.inherited` above).
@config ||= __config_build__
end

Expand Down
4 changes: 3 additions & 1 deletion lib/dry/configurable/compiler.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,9 @@ def visit_setting(node)
# @api private
def visit_nested(node)
parent, children = node
visit(parent).nested(call(children))
name, opts = parent[1]

Setting.new(name, **opts, children: Settings.new(call(children)))
Comment on lines -32 to +34
Copy link
Member Author

@timriley timriley Sep 13, 2022

Choose a reason for hiding this comment

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

To save ourselves from creating a throwaway object (i.e. a Setting that we initialize and then have to re-initialize with the children), we build a single Setting instance here directly rather than running its part of the AST back through the compiler.

end
end
end
Expand Down
101 changes: 68 additions & 33 deletions lib/dry/configurable/config.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# frozen_string_literal: true

require "concurrent/map"
require "dry/core/constants"

module Dry
module Configurable
Expand All @@ -14,12 +14,17 @@ class Config
attr_reader :_settings

# @api private
attr_reader :_resolved
attr_reader :_values

# @api private
def initialize(settings)
def initialize(settings, values: {})
@_settings = settings
@_resolved = Concurrent::Map.new
@_values = values
end

# @api private
def dup_for_settings(settings)
self.class.new(settings, values: dup_values)
end

# Get config value by a key
Expand All @@ -29,9 +34,12 @@ def initialize(settings)
# @return Config value
def [](name)
name = name.to_sym
raise ArgumentError, "+#{name}+ is not a setting name" unless _settings.key?(name)

_settings[name].value
unless (setting = _settings[name])
raise ArgumentError, "+#{name}+ is not a setting name"
end

_values.fetch(name) { _values[name] = setting.to_value }
end

# Set config value.
Expand All @@ -40,7 +48,15 @@ def [](name)
# @param [String,Symbol] name
# @param [Object] value
def []=(name, value)
public_send(:"#{name}=", value)
raise FrozenConfig, "Cannot modify frozen config" if frozen?

name = name.to_sym

unless (setting = _settings[name])
raise ArgumentError, "+#{name}+ is not a setting name"
end

_values[name] = setting.constructor.(value)
end

# Update config with new values
Expand All @@ -65,61 +81,80 @@ def update(values)
self
end

# Dump config into a hash
# Returns the current config values.
#
# Nested configs remain in their {Config} instances.
#
# @return [Hash]
#
# @api public
def values
_settings
.map { |setting| [setting.name, setting.value] }
.map { |key, value| [key, value.is_a?(self.class) ? value.to_h : value] }
.to_h
# Ensure all settings are represented in values
_settings.each { |setting| self[setting.name] unless _values.key?(setting.name) }

_values
end

# Returns config values as a hash, with nested values also converted from {Config} instances
# into hashes.
#
# @return [Hash]
#
# @api public
def to_h
values.map { |key, value| [key, value.is_a?(self.class) ? value.to_h : value] }.to_h
timriley marked this conversation as resolved.
Show resolved Hide resolved
end
alias_method :to_h, :values

# @api private
def finalize!(freeze_values: false)
_settings.finalize!(freeze_values: freeze_values)
values.each_value do |value|
if value.is_a?(self.class)
value.finalize!(freeze_values: freeze_values)
elsif freeze_values
value.freeze
end
end

freeze
end

# @api private
def pristine
self.class.new(_settings.pristine)
end

# @api private
def respond_to_missing?(meth, include_private = false)
super || _settings.key?(resolve(meth))
self.class.new(_settings)
end

private

# @api private
def method_missing(meth, *args)
setting = _settings[resolve(meth)]
def method_missing(name, *args)
setting_name = setting_name_from_method(name)
setting = _settings[setting_name]

super unless setting

if setting.writer?(meth)
raise FrozenConfig, "Cannot modify frozen config" if frozen?

_settings << setting.with(input: args[0])
if name.end_with?("=")
self[setting_name] = args[0]
Comment on lines -105 to +135
Copy link
Member Author

Choose a reason for hiding this comment

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

Clarified reading and writing of values here by moving all such logic into the #[] and #[]= methods, rather than it being spread between them and #method_missing.

else
setting.value
self[setting_name]
end
end

# @api private
def resolve(meth)
_resolved.fetch(meth) { _resolved[meth] = meth.to_s.tr("=", "").to_sym }
def respond_to_missing?(meth, include_private = false)
_settings.key?(setting_name_from_method(meth)) || super
end

def setting_name_from_method(method_name)
method_name.to_s.tr("=", "").to_sym
end

def dup_values
_values.each_with_object({}) { |(key, val), dup_hsh|
dup_hsh[key] = _settings[key].cloneable? ? val.dup : val
}
end

# @api private
def initialize_copy(source)
super
@_settings = source._settings.dup
@_values = source.send(:dup_values)
timriley marked this conversation as resolved.
Show resolved Hide resolved
end
end
end
Expand Down
5 changes: 0 additions & 5 deletions lib/dry/configurable/dsl.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,12 @@ module Configurable
class DSL
VALID_NAME = /\A[a-z_]\w*\z/i.freeze

# @api private
Copy link
Member Author

Choose a reason for hiding this comment

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

This whole class is marked as # @api private so these lines aren't needed.

attr_reader :compiler

# @api private
attr_reader :ast

# @api private
attr_reader :options

# @api private
def initialize(**options, &block)
@compiler = Compiler.new
@ast = []
Expand Down Expand Up @@ -141,7 +137,6 @@ def setting(name, default = Undefined, **options, &block) # rubocop:disable Metr
compiler.visit(ast.last)
end

# @api private
def config_class
options[:config_class]
end
Expand Down
5 changes: 1 addition & 4 deletions lib/dry/configurable/instance_methods.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,7 @@ module Configurable
module Initializer
# @api private
def initialize(*)
# Dup settings at time of initializing to ensure setting values are specific to
# this instance. This does mean that any settings defined on the class _after_
# initialization will not be available on the instance.
@config = self.class.__config_build__(self.class._settings.dup)
@config = self.class.__config_build__(self.class._settings)

super
end
Expand Down
Loading