Add Rails::Application#config_for #16129

Merged
merged 1 commit into from Jul 15, 2014

Conversation

Projects
None yet
8 participants
@rafaelfranca
Member

rafaelfranca commented Jul 10, 2014

This is a convenience for loading configuration for the current Rails
environment.

production:
  url: http://127.0.0.1:8080
  namespace: bc3_production
development:
  url: http://bc3.dev/cleversafe
  namespace: bc3_development
Cleversafe.config = Application.config_for :cleversafe

cc @dhh

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Jul 10, 2014

Member

Might use a better example, like:

# config/production.rb
BCX::Application.configure do
  config.middleware.use ExceptionNotifier, config_for(:exception_notification)
end

Otherwise looks good!

Member

dhh commented Jul 10, 2014

Might use a better example, like:

# config/production.rb
BCX::Application.configure do
  config.middleware.use ExceptionNotifier, config_for(:exception_notification)
end

Otherwise looks good!

+ test "config_for loads custom configuration from yaml files" do
+ app_file 'config/custom.yml', <<-RUBY
+ development:
+ custom: 'custom value'

This comment has been minimized.

@matthewd

matthewd Jul 10, 2014

Member

Might be clearer to read if this was called something else? As it is, the test later uses 'custom' in two places to refer to different things...

@matthewd

matthewd Jul 10, 2014

Member

Might be clearer to read if this was called something else? As it is, the test later uses 'custom' in two places to refer to different things...

@egilburg

This comment has been minimized.

Show comment
Hide comment
@egilburg

egilburg Jul 10, 2014

Contributor

If Rails will provide canonical way to manage env-specific configuration from yaml, should it be in scope to have inheritable application yaml configuration, overridable per environment, similar to how the ruby config in environment.rb/application.rb is inherited and overridable by environments/foo.rb?

Contributor

egilburg commented Jul 10, 2014

If Rails will provide canonical way to manage env-specific configuration from yaml, should it be in scope to have inheritable application yaml configuration, overridable per environment, similar to how the ruby config in environment.rb/application.rb is inherited and overridable by environments/foo.rb?

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jul 10, 2014

Member

@egilburg not sure if I follow. Is not YAML inheritance sufficient?

development: &default
  foo:
  bar:

production:
  <<*default
  bar:
Member

rafaelfranca commented Jul 10, 2014

@egilburg not sure if I follow. Is not YAML inheritance sufficient?

development: &default
  foo:
  bar:

production:
  <<*default
  bar:
@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Jul 10, 2014

Member

I haven’t found that helpful or useful for my usage of this feature. These config files are usually pretty small, and it gives more clarity to have the env declarations together.

On Jul 10, 2014, at 3:59 PM, Eugene Gilburg notifications@github.com wrote:

If Rails will provide canonical way to manage env-specific configuration from yaml, should it be in scope to have inheritable application yaml configuration, overridable per environment, similar to how the ruby config in environment.rb is inherited and overridable by environments/foo.rb?


Reply to this email directly or view it on GitHub.

Member

dhh commented Jul 10, 2014

I haven’t found that helpful or useful for my usage of this feature. These config files are usually pretty small, and it gives more clarity to have the env declarations together.

On Jul 10, 2014, at 3:59 PM, Eugene Gilburg notifications@github.com wrote:

If Rails will provide canonical way to manage env-specific configuration from yaml, should it be in scope to have inheritable application yaml configuration, overridable per environment, similar to how the ruby config in environment.rb is inherited and overridable by environments/foo.rb?


Reply to this email directly or view it on GitHub.

@egilburg

This comment has been minimized.

Show comment
Hide comment
@egilburg

egilburg Jul 10, 2014

Contributor

@rafaelfranca, sure, but this is a bit of sugar to match the sugar we get from the Ruby config. So instead of:

  default: &default
    foo: 'bar'
  production:
    << &default
    baz: 'production_baz'

We'd have:

  application:
    foo: 'bar'
  production:
    baz: 'production_baz'

It only saves a few lines, but the idea that it follows the pattern Rails uses where environment/production.rb inherits everything in application.rb, helping to group concerns together.

Contributor

egilburg commented Jul 10, 2014

@rafaelfranca, sure, but this is a bit of sugar to match the sugar we get from the Ruby config. So instead of:

  default: &default
    foo: 'bar'
  production:
    << &default
    baz: 'production_baz'

We'd have:

  application:
    foo: 'bar'
  production:
    baz: 'production_baz'

It only saves a few lines, but the idea that it follows the pattern Rails uses where environment/production.rb inherits everything in application.rb, helping to group concerns together.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jul 10, 2014

Member

I really believe we should not try to include new features on yaml parsing 😄

Member

rafaelfranca commented Jul 10, 2014

I really believe we should not try to include new features on yaml parsing 😄

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Jul 10, 2014

Member

Ah, Eugene, I see what you mean. I thought you wanted separate files per env.

I’m with Rafael, though. YAML is hairy enough as it is. I don’t think we want to add to that.

On Jul 10, 2014, at 4:04 PM, Eugene Gilburg notifications@github.com wrote:

@rafaelfranca, sure, but this is a bit of sugar to match the sugar we get from the Ruby config. So instead of:

default:

foo: 'bar'

production:

<< &default

baz: 'production_baz'
We'd have:

application:

foo: 'bar'

production:

baz: 'production_baz'
It only saves a few lines, but the idea that it follows the pattern Rails uses where environment/production.rb inherits everything in application.rb, helping to group concerns together.


Reply to this email directly or view it on GitHub.

Member

dhh commented Jul 10, 2014

Ah, Eugene, I see what you mean. I thought you wanted separate files per env.

I’m with Rafael, though. YAML is hairy enough as it is. I don’t think we want to add to that.

On Jul 10, 2014, at 4:04 PM, Eugene Gilburg notifications@github.com wrote:

@rafaelfranca, sure, but this is a bit of sugar to match the sugar we get from the Ruby config. So instead of:

default:

foo: 'bar'

production:

<< &default

baz: 'production_baz'
We'd have:

application:

foo: 'bar'

production:

baz: 'production_baz'
It only saves a few lines, but the idea that it follows the pattern Rails uses where environment/production.rb inherits everything in application.rb, helping to group concerns together.


Reply to this email directly or view it on GitHub.

@guilleiguaran

This comment has been minimized.

Show comment
Hide comment
@guilleiguaran

guilleiguaran Jul 10, 2014

Member

+1 to @dhh @rafaelfranca please keep yaml parsing as is 😅

Member

guilleiguaran commented Jul 10, 2014

+1 to @dhh @rafaelfranca please keep yaml parsing as is 😅

@egilburg

This comment has been minimized.

Show comment
Hide comment
@egilburg

egilburg Jul 10, 2014

Contributor

This wouldn't be so much parsing as merging the already parsed values:

  data = YAML.load(ERB.new(yaml.read).result) || {}
  data['application'] || {}).merge(data[Rails.env] || {}).presence

If it's not considered useful from behavior perspective, sure. Just pointing out we don't need to change any of the actual Yaml parsing itself.

Contributor

egilburg commented Jul 10, 2014

This wouldn't be so much parsing as merging the already parsed values:

  data = YAML.load(ERB.new(yaml.read).result) || {}
  data['application'] || {}).merge(data[Rails.env] || {}).presence

If it's not considered useful from behavior perspective, sure. Just pointing out we don't need to change any of the actual Yaml parsing itself.

@egilburg

This comment has been minimized.

Show comment
Hide comment
@egilburg

egilburg Jul 10, 2014

Contributor

Also, if yaml has no env key defined, it seems current implementation returns nil. Should it return empty hash instead to behave more like a null object? Otherwise users must remember to always define all environment keys even if they only need config for some environments.

Contributor

egilburg commented Jul 10, 2014

Also, if yaml has no env key defined, it seems current implementation returns nil. Should it return empty hash instead to behave more like a null object? Otherwise users must remember to always define all environment keys even if they only need config for some environments.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jul 10, 2014

Member

This would lead some questions like: Why database.yml doesn't work this way? Why it doesn't work when I don't use config_for? Why it only work inside Rails?

I think we should stick with what yaml give to us.

About the nil return my first implementation was returning {}, so I'm fine to changing it to always return a Hash.

Member

rafaelfranca commented Jul 10, 2014

This would lead some questions like: Why database.yml doesn't work this way? Why it doesn't work when I don't use config_for? Why it only work inside Rails?

I think we should stick with what yaml give to us.

About the nil return my first implementation was returning {}, so I'm fine to changing it to always return a Hash.

+ require "#{app_path}/config/environment"
+ end
+
+ assert_equal "Could not load configuration. No such file - #{app_path}/config/custom.yml" ,exception.message

This comment has been minimized.

@robin850

robin850 Jul 13, 2014

Member

Nitpicking but the comma's place is wrong.

@robin850

robin850 Jul 13, 2014

Member

Nitpicking but the comma's place is wrong.

@robin850

This comment has been minimized.

Show comment
Hide comment
@robin850

robin850 Jul 13, 2014

Member

Nice patch! 👍 Maybe we can also simplify Rails::Application#secrets definition with this feature, no ?

Member

robin850 commented Jul 13, 2014

Nice patch! 👍 Maybe we can also simplify Rails::Application#secrets definition with this feature, no ?

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jul 15, 2014

Member

I can't simplify secrets because the behaviour when the secrets.yml files doesn't exist is different from when a generic config file doesn't exist. To use config_for in secrets I'd had to raise an specific exception when the file doesn't exist and catching on secrets. I believe it is not worth to remove this duplication.

Member

rafaelfranca commented Jul 15, 2014

I can't simplify secrets because the behaviour when the secrets.yml files doesn't exist is different from when a generic config file doesn't exist. To use config_for in secrets I'd had to raise an specific exception when the file doesn't exist and catching on secrets. I believe it is not worth to remove this duplication.

Add Rails::Application#config_for
This is a convenience for loading configuration for the current Rails
environment.

rafaelfranca added a commit that referenced this pull request Jul 15, 2014

Merge pull request #16129 from rafaelfranca/config_for
Add Rails::Application#config_for

@rafaelfranca rafaelfranca merged commit 1fbb5c1 into rails:master Jul 15, 2014

@rafaelfranca rafaelfranca deleted the rafaelfranca:config_for branch Jul 15, 2014

@pabloh

This comment has been minimized.

Show comment
Hide comment
@pabloh

pabloh Aug 29, 2014

Contributor

No dogfooding?

Contributor

pabloh commented on 9629dea Aug 29, 2014

No dogfooding?

@pseudomuto pseudomuto referenced this pull request in pseudocms/app-api Sep 1, 2014

Merged

Upgrading to Rails 4.2.0.beta1 #14

+ require "erb"
+ (YAML.load(ERB.new(yaml.read).result) || {})[Rails.env] || {}
+ else
+ raise "Could not load configuration. No such file - #{yaml}"

This comment has been minimized.

@mdeering

mdeering Feb 16, 2015

Any reason behind raising the default RuntimeError VS something more specific such as IOError or reraising the Errno::ENOENT?

@mdeering

mdeering Feb 16, 2015

Any reason behind raising the default RuntimeError VS something more specific such as IOError or reraising the Errno::ENOENT?

This comment has been minimized.

@dhh

dhh Feb 16, 2015

Member

Nope, fine to raise re-raise, as long as we use that new message.

On Feb 16, 2015, at 10:35, Michael Deering notifications@github.com wrote:

In railties/lib/rails/application.rb:

  • url: http://localhost:3001

  • namespace: my_app_development

  • # config/production.rb

  • MyApp::Application.configure do

  • config.middleware.use ExceptionNotifier, config_for(:exception_notification)

  • end

  • def config_for(name)
  •  yaml = Pathname.new("#{paths["config"].existent.first}/#{name}.yml")
    
  •  if yaml.exist?
    
  •    require "yaml"
    
  •    require "erb"
    
  •    (YAML.load(ERB.new(yaml.read).result) || {})[Rails.env] || {}
    
  •  else
    
  •    raise "Could not load configuration. No such file - #{yaml}"
    

Any reason behind raising the default RuntimeError VS something more specific such as IOError or reraising the Errno::ENOENT?


Reply to this email directly or view it on GitHub.

@dhh

dhh Feb 16, 2015

Member

Nope, fine to raise re-raise, as long as we use that new message.

On Feb 16, 2015, at 10:35, Michael Deering notifications@github.com wrote:

In railties/lib/rails/application.rb:

  • url: http://localhost:3001

  • namespace: my_app_development

  • # config/production.rb

  • MyApp::Application.configure do

  • config.middleware.use ExceptionNotifier, config_for(:exception_notification)

  • end

  • def config_for(name)
  •  yaml = Pathname.new("#{paths["config"].existent.first}/#{name}.yml")
    
  •  if yaml.exist?
    
  •    require "yaml"
    
  •    require "erb"
    
  •    (YAML.load(ERB.new(yaml.read).result) || {})[Rails.env] || {}
    
  •  else
    
  •    raise "Could not load configuration. No such file - #{yaml}"
    

Any reason behind raising the default RuntimeError VS something more specific such as IOError or reraising the Errno::ENOENT?


Reply to this email directly or view it on GitHub.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment