-
-
Notifications
You must be signed in to change notification settings - Fork 234
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
Settings are loaded before setup
block executed
#244
Comments
Reproduced this in an example app https://github.com/supremebeing7/example_config |
Worth mentioning also that the |
OK, I think there's two issues here:
initializer = ::Rails.root.join('config', 'initializers', 'config.rb')
require initializer if File.exist?(initializer)
Once I noticed the railtie from [1] above, then [2] makes sense, since the I'm not sure the best solution here... I do think some additional documentation around this would be helpful. However, I'm also wondering if maybe the @pkuczynski What do you think? I'm happy to work on this, just unsure what solution makes the most sense. Also open to other ideas than the ones I laid out. |
Yeah, the whole point of the initializer is to define configuration options for the Config before the instance is created. The reason it's done this way was, that you don't need an initializer at all if you follow default setup, as Config is plugging into the rails automatically here: https://github.com/railsconfig/config/blob/master/lib/config.rb#L81 Also see my comment in #187 Question is: why would you like to access Maybe we should update documentation. PR welcome :) |
Thanks for the clarification @pkuczynski.
Makes sense! I think the default setup is great and wouldn't want to change how this works.
In my specific case, it was due to migrating from a different configuration gem where we had assigned config values to a constant I'll be the first to admit that's probably not a common use case. A perhaps more common use case might be the With the caveat that I'm not too familiar with how other projects like this are structured, to me it would make more sense to move the setup block to Obviously this would have to wait for the next minor version at least since it's a breaking change, though we could implement both approaches now and show a deprecation warning for the initializer approach to help ease the transition. I'm happy to work on this if that all seems reasonable. However if we think that's excessive or unnecessary and we just want to add more documentation around this, I can certainly do that instead. |
As I mentioned the other day, I am mainly working on Node.js stack these days, so can't really say how modern app layout is done nowadays. Adding some more explanation to the @rdubya @pyromaniac what do you think about changing the behavior? I am personally not sure if that's worth the hassle... |
It does seem like it would make sense to move it out of the initializers directory. We reference Settings in other initializers but not in the config initializer. |
Then sure, we can work on this and release as 2.1. @supremebeing7 if you wanna take it over, go ahead. |
@supremebeing7 are u still interested in firing up a PR? |
@pkuczynski I'm interested but unfortunately don't have time currently. So if someone else wants to take a stab at it, they can. Otherwise we can push it to a later milestone and I can look into it then. |
@supremebeing7 are you still interested in fixing this? |
@pkuczynski Interested, but I don't have time to do it. And I don't see that situation changing anytime soon. |
That's a shame, but I totally understand. Will keep this open and ping you in some time :) |
@supremebeing7 how do you look with time now? :) |
@pkuczynski Still unavailable to work on this. FWIW: This is very low priority for me - after the initial setup, this issue has not come up again. So, unless someone else wants to pick this up, I'd be fine with closing it. |
Got it! @cjlarose do you wanna have a look or shall we close it? |
I can take a look. |
good job sir |
Taking another look at this recently since #352 is related. This issue brings up a couple of distinct, though related, points
I think the solution is still the same: let's move configuration for the |
Version 2.0.0
I'm trying to use the
use_env
setting, but was noticing the override wasn't working. On closer inspection, it appears as if all of my settings are loaded prior to theConfig.setup
block being executed. I verified this by checking it in the initializer:Has anyone else experienced this? I don't think it's due to any special setup for my particular app, but will try to spin up an example app to test.
The text was updated successfully, but these errors were encountered: