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

DRY - Code Refactor #310

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all 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
20 changes: 10 additions & 10 deletions lib/config/integrations/heroku.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,16 +41,16 @@ def `(command)
def to_dotted_hash(source, target = {}, namespace = nil)
prefix = "#{namespace}." if namespace
case source
when Hash
source.each do |key, value|
to_dotted_hash(value, target, "#{prefix}#{key}")
end
when Array
source.each_with_index do |value, index|
to_dotted_hash(value, target, "#{prefix}#{index}")
end
else
target[namespace] = source
when Hash
source.each do |key, value|
to_dotted_hash(value, target, "#{prefix}#{key}")
end
when Array
source.each_with_index do |value, index|
to_dotted_hash(value, target, "#{prefix}#{index}")
end
else
target[namespace] = source
end
target
end
Expand Down
88 changes: 40 additions & 48 deletions lib/config/options.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,20 +15,11 @@ def empty?
end

def add_source!(source)
# handle yaml file paths
source = (Sources::YAMLSource.new(source)) if source.is_a?(String)
source = (Sources::HashSource.new(source)) if source.is_a?(Hash)

@config_sources ||= []
@config_sources << source
load_config_sources(:push, source)
end

def prepend_source!(source)
source = (Sources::YAMLSource.new(source)) if source.is_a?(String)
source = (Sources::HashSource.new(source)) if source.is_a?(Hash)

@config_sources ||= []
@config_sources.unshift(source)
load_config_sources(:unshift, source)
end

# look through all our sources and rebuild the configuration
Expand All @@ -40,15 +31,7 @@ def reload!
if conf.empty?
conf = source_conf
else
DeepMerge.deep_merge!(
source_conf,
conf,
preserve_unmergeables: false,
knockout_prefix: Config.knockout_prefix,
overwrite_arrays: Config.overwrite_arrays,
merge_nil_values: Config.merge_nil_values,
merge_hash_arrays: Config.merge_hash_arrays
)
hash_deep_merge(source_conf, conf)
end
end

Expand All @@ -69,14 +52,8 @@ def reload_from_files(*files)

def to_hash
result = {}
marshal_dump.each do |k, v|
if v.instance_of? Config::Options
result[k] = v.to_hash
elsif v.instance_of? Array
result[k] = descend_array(v)
else
result[k] = v
end
marshal_dump.each do |key, value|
result[key] = instance_of(value)
end
result
end
Expand All @@ -98,15 +75,7 @@ def as_json(options = nil)

def merge!(hash)
current = to_hash
DeepMerge.deep_merge!(
hash.dup,
current,
preserve_unmergeables: false,
knockout_prefix: Config.knockout_prefix,
overwrite_arrays: Config.overwrite_arrays,
merge_nil_values: Config.merge_nil_values,
merge_hash_arrays: Config.merge_hash_arrays
)
hash_deep_merge(hash.dup, current)
marshal_load(__convert(current).marshal_dump)
self
end
Expand Down Expand Up @@ -152,23 +121,46 @@ def respond_to_missing?(*args)

protected

def descend_array(array)
array.map do |value|
if value.instance_of? Config::Options
value.to_hash
elsif value.instance_of? Array
descend_array(value)
else
value
end
def load_config_sources(method, source)
# handle yaml file paths
source = (Sources::YAMLSource.new(source)) if source.is_a?(String)
source = (Sources::HashSource.new(source)) if source.is_a?(Hash)

@config_sources ||= []
@config_sources.send(method, source)
end

def hash_deep_merge(source, dest)
DeepMerge.deep_merge!(
source,
dest,
preserve_unmergeables: false,
knockout_prefix: Config.knockout_prefix,
overwrite_arrays: Config.overwrite_arrays,
merge_nil_values: Config.merge_nil_values,
merge_hash_arrays: Config.merge_hash_arrays
)
end

def instance_of(value)
Copy link
Member

@Fryguy Fryguy Jan 12, 2022

Choose a reason for hiding this comment

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

instance_of is a strange name here, but I'm struggling coming up with a better name. To me, instance_of implies you are getting another instance of something (perhaps a dup), but it's actually returning the converted value of the passed in value. Perhaps

Suggested change
def instance_of(value)
def serialize_value(value)

Once that is changed, I was thinking you can also rename descend_array to serialize_array to match the pattern, but now that I look at it, there's no longer a reason to have a separate method for handling an array, and it can probably be inlined directly into this method.

if value.instance_of? Config::Options
value.to_hash
elsif value.instance_of? Array
descend_array(value)
else
value
end
end

def descend_array(array)
array.map { |value| instance_of(value) }
end

# Recursively converts Hashes to Options (including Hashes inside Arrays)
def __convert(h) #:nodoc:
def __convert(hash) #:nodoc:
s = self.class.new

h.each do |k, v|
hash.each do |k, v|
k = k.to_s if !k.respond_to?(:to_sym) && k.respond_to?(:to_s)

if v.is_a?(Hash)
Expand Down
4 changes: 2 additions & 2 deletions lib/config/rack/reloader.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
module Config
module Rack
# Rack middleware the reloads Config on every request (only use in dev mode)
# Rack middleware that reloads Config on every request (only use in dev mode)
class Reloader
def initialize(app)
@app = app
Expand All @@ -12,4 +12,4 @@ def call(env)
end
end
end
end
end
12 changes: 6 additions & 6 deletions lib/config/sources/env_source.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,12 @@ def load

keys.map! { |key|
case converter
when :downcase then
key.downcase
when nil then
key
else
raise "Invalid ENV variables name converter: #{converter}"
when :downcase
key.downcase
when nil
key
else
raise "Invalid ENV variables name converter: #{converter}"
end
}

Expand Down
2 changes: 1 addition & 1 deletion lib/config/sources/yaml_source.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ class YAMLSource

def initialize(path, evaluate_erb: Config.evaluate_erb_in_yaml)
@path = path.to_s
@evaluate_erb = !!evaluate_erb
@evaluate_erb = evaluate_erb
Copy link
Member

Choose a reason for hiding this comment

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

Not sure about this one...it's kind of nice having a proper boolean, since it is accessible via the attr_reader.

Copy link
Member

Choose a reason for hiding this comment

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

I'm on the same page as you @Fryguy . Several of the changes proposed in this PR compromise correctness or readability in favor of succinctness.

I'm comfortable with the serialize_value suggestion as a separate method as well as the formatting for case/when but I'm not excited about the other changes.

end

# returns a config hash from the YML file
Expand Down