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. On Thu, Oct 29, 2015 at 1:33 PM Chris Nicola <[email protected]> wrote: > 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 > > > > On Wednesday, 28 October 2015 13:07:25 UTC-7, Rafael Mendonça França wrote: > >> 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. >> >> >> On Wed, Oct 28, 2015 at 5:38 PM Chris Nicola <[email protected]> wrote: >> > 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. >>> >>> -- >>> 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 [email protected]. >>> To post to this group, send email to [email protected]. >> >> >>> Visit this group at http://groups.google.com/group/rubyonrails-core. >>> For more options, visit https://groups.google.com/d/optout. >>> >> -- > 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 [email protected]. > To post to this group, send email to [email protected]. > Visit this group at http://groups.google.com/group/rubyonrails-core. > For more options, visit https://groups.google.com/d/optout. > -- 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 [email protected]. To post to this group, send email to [email protected]. Visit this group at http://groups.google.com/group/rubyonrails-core. For more options, visit https://groups.google.com/d/optout.
