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.

Sign up for GitHub

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

Issue#1243 configurations with unimplemented environments #1244

Merged
merged 1 commit into from
Mar 22, 2018
Merged

Issue#1243 configurations with unimplemented environments #1244

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
Load current environment settings regardless of support for other env… 
…ironments in settings file.
  • Loading branch information
@JonMidhir
JonMidhir committed Mar 20, 2018
commit dac516088e9a8279dd7d9a2e079ff23e4ffab5b1
60 changes: 30 additions & 30 deletions sinatra-contrib/lib/sinatra/config_file.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ module Sinatra
#
# <tt>Sinatra::ConfigFile</tt> is an extension that allows you to load the
# application's configuration from YAML files. It automatically detects if
# the files contains specific environment settings and it will use the
# the files contain specific environment settings and it will use those
# corresponding to the current one.
#
# You can access those options through +settings+ within the application. If
Expand Down Expand Up @@ -94,18 +94,11 @@ module Sinatra
# In either case, <tt>settings.foo</tt> will return the environment name, and
# <tt>settings.bar</tt> will return <tt>"bar"</tt>.
#
# Be aware that if you have a different environment, besides development,
# test and production, you will also need to adjust the +environments+
# setting, otherwise the settings will not load. For instance, when
# you also have a staging environment:
# If you wish to provide defaults that may be shared among all the
# environments, this can be done by using a YAML alias, and then overwriting
# values in environments where appropriate:
#
# set :environments, %w{development test production staging}
#
# If you wish to provide defaults that may be shared among all the environments,
# this can be done by using one of the existing environments as the default using
# the YAML alias, and then overwriting values in the other environments:
#
# development: &common_settings
# default: &common_settings
# foo: 'foo'
# bar: 'bar'
#
Expand All @@ -131,11 +124,9 @@ def config_file(*paths)
raise UnsupportedConfigType unless ['.yml', '.erb'].include?(File.extname(file))
logger.info "loading config file '#{file}'" if logging? && respond_to?(:logger)
document = ERB.new(IO.read(file)).result
yaml = config_for_env(YAML.load(document)) || {}
yaml.each_pair do |key, value|
for_env = config_for_env(value)
set key, for_env unless value and for_env.nil? and respond_to? key
end
yaml = YAML.load(document)
config = config_for_env(yaml)
config.each_pair { |key, value| set(key, value) }
Copy link
Member

Choose a reason for hiding this comment

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

You shouldn't remove the conditional expressions because the following unintentional behavior will happen.

# config.yml
development:
  raise_errors: "foo"
require 'sinatra'
require 'sinatra/config_file'

config_file './config.yml'

get '/' do
  settings.raise_errors.to_s #=> expected "false", but got "foo"
end

I think that it isn't desirable to automatically overwrite settings defined by default by config file settings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@namusyaka Running this against both master and this branch I notice the default setting being overwritten. I don't think the original conditional defends against this behaviour. I can't actually think of a condition where the original would be met at all.

However, I can add this behaviour if you'd prefer?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, makes sense.
It seems that the sample I showed was wrong.
However, we must grasp the condition that the original conditional expression was trying to protect.

Welcome if you can look it up.
Then, if the original conditional expression is meaningful, we have to add a test on that matter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have the case where this would fail. If the value is being set to nil and it's set using this style, the conditional will be met and setting ignored:

# config.yml
raise_errors:
  development: 

# app.rb
settings.raise_errors #=> expected nil, got false

However in both other styles the conditional is not met, and the default value is overwritten:

# config.yml
raise_errors:

# app.rb
settings.raise_errors #=> expected nil, got nil



# config.yml
development:
  raise_errors:

# app.rb
settings.raise_errors #=> expected nil, got nil

If we're changing the behaviour anyway do we really want to stop people from overriding default values using the config file? It seems reasonable that it should be possible to do so.

If not we can exclude all cases or have it function exactly as it did before.

Copy link
Member

Choose a reason for hiding this comment

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

It's no problem, I think.
I've reviewed this change to a certain extent, but I judged that it is almost no problem.

end
end
end
Expand All @@ -149,23 +140,32 @@ def message

private

# Given a +hash+ with some application configuration it returns the
# settings applicable to the current environment. Note that this can only
# be done when all the keys of +hash+ are environment names included in the
# +environments+ setting (which is an Array of Strings). Also, the
# returned config is a indifferently accessible Hash, which means that you
# can get its values using Strings or Symbols as keys.
# Given a +hash+ containing application configuration it returns
# settings applicable to the current environment. Note: It gives
# precedence to environment settings defined at the root-level.
def config_for_env(hash)
if hash.respond_to?(:keys) && hash.keys.all? { |k| environments.include?(k.to_s) }
hash = hash[environment.to_s] || hash[environment.to_sym]
end
return from_environment_key(hash) if environment_keys?(hash)

if hash.respond_to?(:to_hash)
IndifferentHash[hash.to_hash]
else
hash
hash.each_with_object(IndifferentHash[]) do |(k, v), acc|
if environment_keys?(v)
acc.merge!(k => v[environment.to_s]) if v.key?(environment.to_s)
else
acc.merge!(k => v)
end
end
end

# Given a +hash+ returns the settings corresponding to the current
# environment.
def from_environment_key(hash)
hash[environment.to_s] || hash[environment.to_sym] || {}
end

# Returns true if supplied with a hash that has any recognized
# +environments+ in its root keys.
def environment_keys?(hash)
hash.is_a?(Hash) && hash.any? { |k, _| environments.include?(k.to_s) }
end
end

register ConfigFile
Expand Down
16 changes: 16 additions & 0 deletions sinatra-contrib/spec/config_file/with_env_defaults.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
---
default: &default
foo: default
bar: baz

development:
<<: *default
foo: development

production:
<<: *default
foo: production

test:
<<: *default
foo: test
20 changes: 20 additions & 0 deletions sinatra-contrib/spec/config_file_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,4 +73,24 @@ def config_file(*args, &block)
config_file 'key_value_override.yml'
expect(settings.foo).to eq('foo')
end

context 'when file contains superfluous environments' do
before { config_file 'with_env_defaults.yml' }

it 'loads settings for the current environment anyway' do
expect { settings.foo }.not_to raise_error
end
end

context 'when file contains defaults' do
before { config_file 'with_env_defaults.yml' }

it 'uses the overridden value' do
expect(settings.foo).to eq('test')
end

it 'uses the default value' do
expect(settings.bar).to eq('baz')
end
end
end