Suggesting an alternative to raising a file missing exception in `config_for`

[Chris Nicola]

Currently `Application#config_for` raises an exception whenever a config file isn't found. However, this doesn't really seem very exceptional to me and there are going to be a lot of times where I want to configure an application module only if the configuration is available and something else if it isn't.

I could of course check if the file exists on my own, but then that almost entirely defeats the convenience of this convenience function.

A few possible solutions:

#### Offer an easy way to check if the file exists like `config_exists?`

I don't like this because it feels too much like a nil checking pattern.

#### Allow config_for to accept a block or argument

This is similar to the `Hash#fetch` pattern, and the value of the block can be returned to the caller. The default exception behavior can remain if no value or block is provided. This would also allow for a few patterns.

1. Provide alternative configuration logic when the file isn't found

2. Don't configure the component when the file isn't found and log a warning

For example:

```

if config_for(:the_thing, false)

  # configure the thing

else

  # don't configure the thing

  logger.warn "The thing is not configured, no configuration file is found

end

```

I'm definitely open to other suggestions, but the way `config_for` works right now I'm very hesitant to use it.

[Rafael Mendonça França]

I see the file not being there an exceptional case. Making config_for not raise if the file doesn't exists can lead to more problems like not reading the config file because of a typo in the file name.

```

if config_for(:rdis)

  # configure the thing

else

  # don't configure the thing

end

```

The block version would be weird too:

```

redis_config = config_for(:redis) do

  # don't configure the thing

end

Redis.config = redis_config

```

The only solution that would work is the helper to check the file existence.

--
You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-co...@googlegroups.com.
To post to this group, send email to rubyonra...@googlegroups.com.
Visit this group at http://groups.google.com/group/rubyonrails-core.
For more options, visit https://groups.google.com/d/optout.

[Chris Nicola]

I can't really agree on the exceptionality of not including a config file, my preference in most cases would be to WARN as most gems for third party services do (e.g. NewRelic doesn't prevent my server from starting up if it is missing it's configuration). I also want to make sure I understand this, we want to prevent the server from starting if the file isn't there and we want this to be the only option outside of manually checking for it's existence?

In the end what I'm suggesting is that this developers should not be limited in how they can handle a missing config. Providing a helper would some kind of solution, but I don't see why it would be preferred over an idiomatic solution similar to Hash#fetch. I would further suggest that it suffers from the same problem of a typo issue, as well as being less "DRY" since you have to repeat the name of the file. For example:

if has_config_for? :rdis do

  config.x.redis = config_for(:redis)

else

  config.logger.warn("Redis config is missing")

end

This is now misleading as it isn't redis.yml that is missing, but rdis.yml.

The block approach having a couple of benefits over just checking existance.

1) If a block or value is not provided as the second argument it can basically keep the exact same logic (raising an exception) as before. No change to any existing usages.

2) It can be made DRYer since we only reference the config file name once and we could pass the name into the block if we want to allowing for a generic helper that does something like logging a missing file warning or error.

# /config/initializers/redis.rb

redis_config = Rails.application.config_for(:redis) do |config_name|

  config.logger.warn("Configuration for #{config_name} not found!")

  false

end

return unless redis_config

3) It feels more idiomatic to me as this is how Hash#fetch already works

4) An existence checking helper still feels like basically just another form of nil checking in the end

[Rafael Mendonça França]

The downside of the fetch approach is that you will need to make sure the block return false an you will have to check the value existence too.

If that is fine to you, I'm fine with it and I agree it is better than having to type the config name twice.

[Chris Nicola]

This is true, the approach is arguably similar, in terms of checking, though it does offer more flexibility in how it is done. My basic example is probably a bit simplistic.

I would also suggest allowing the second parameter to be a straight return value instead of a block, to be returned if the config file is missing.

And I completely agree that the most important thing is to preserve the current behaviour for people who do want to fail fast.

I’m happy to submit a PR if that helps at all.

-- 
Chris Nicola
http://about.me/chrisnicola

You received this message because you are subscribed to a topic in the Google Groups "Ruby on Rails: Core" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/rubyonrails-core/3kCxt2wr8XM/unsubscribe.
To unsubscribe from this group and all its topics, send an email to rubyonrails-co...@googlegroups.com.