Open mikkoi opened 2 years ago
I think calculating location
and environment
only once and transferring it onwards is a good call. I like that.
I think the usage of roles here might be too much. How about:
Dancer2::ConfigReader
as an object holding configuration information.config_readers
who all consume the Role::ConfigReader
role.ConfigReader::File::Simple
(or Config::Type::File::Simple
) will have config_files
as an attribute since it reads from [simple] configuration files. A different configuration might have elastic_params
because it uses ElasticSearch to include configurations, for example.App
would create an instance of the ConfigReader
object with the location and environment.ConfigReader
will determine which configuration reader it should use and instantiate them and use them to collect the configuration.Two things left in mind:
set
keyword, will we be able to use this by the time ConfigReader
tries to instantiate configuration readers?If we ditch config_files
attribute, we create a non-compatible change.
The name is bad but I like the idea that the Config
object would actually know where the configs come from, at least on a surface level (list of files or sources).
If you edit the value of the configuration readers to use using the set keyword, will we be able to use this by the time ConfigReader tries to instantiate configuration readers?
I don't think its possible. For the same reason I couldn't use hooks. It creates a dependency on user code before App is ready - and config should be read and ready before App is accessible for users.
We shouldn't (at least for now) support recursive levels of configuration readers, like "read the config file, then the config file contains other configuration readers, use them, etc." until we research how others handle this problem. (Thought: We do support local config files - so theoretically, we could use that second-level configuration part to hook into reading from non-configuration files...)
Agree. Too many moving parts for the moment.
If we ditch config_files attribute, we create a non-compatible change.
Incompatible with what?
The name is bad but I like the idea that the Config object would actually know where the configs come from, at least on a surface level (list of files or sources).
Sorry, I don't understand your comment here.
Attribute config_files
is part of the public interface, isn't it? If it isn't, then of course, it's not a compatibility issue.
Just thinking that I like the idea that the config readers maintain a list of files or sources where the config is read from.
Attribute
config_files
is part of the public interface, isn't it? If it isn't, then of course, it's not a compatibility issue.
I can't imagine this being used for anything. The Config Reader is a role composed into classes. If someone is using it, that's not in a way that we should worry about officially supporting.
Just thinking that I like the idea that the config readers maintain a list of files or sources where the config is read from.
It makes sense if the specific sense of configuration as files, but in the abstract sense of configuration as values by keys, files are a secondary - and more important, optional - concept.
Attribute
config_files
is part of the public interface, isn't it? If it isn't, then of course, it's not a compatibility issue.
The only thing someone could be using this for, I'd think, is logging the contents of what's there (and that is already happening on app startup). Maybe some devs got clever with the contents. We could remove it and see who complains, or for non-file based configs (etcd, Vault, wherever else you might pull config from) we could put a one-line description in about where the config came from. Personally, I'd rather remove it, make a developer release, document the removal, and see who complains.
Just thinking that I like the idea that the config readers maintain a list of files or sources where the config is read from.
As do I. I am not sure what I'd do with that info yet, but having it is intriguing. If only for some debugging purposes down the road.
I have changed all the small things and I removed the attributeDancer2::Core::Role::Config::config_files
.
@xsawyerx Did you really mean we should make so big change? To abandon App having Config as a role, and instead instantiate ConfigReader? It means we would need to implement the functionality to access configuration content in App and in ConfigReader.
Or else, we could redesign the whole thing so that Config and ConfigReader have nothing to do with each other, except that configuration is something that ConfigReader produces and then gives/assigns to Config.
It seems like a very big change.
I do think we need to make such a big change and I don't think this big change is this "big." Of course, I could be wrong about that. (But it would still be the right thing to do.)
To be on the safe side, I rebased the branch from master. So I had to force push.
The new commits are:
The main changes are: There is now Role::HasConfig which has those parts of the old Role::Config which were about accessing and changing the config. There is now Role::HasEnvironment to isolate the environment deducing code (we get env name from ENV vars, etc.) ConfigReader is instantiated as an attribute to Dancer::Core::App.
The test t/issues/gh-634.t
is failing because it uses config_location
from Role::Config
.
The test needs to be thought out again.
The tests now run. @xsawyerx Do we need more fine tuning before merging?
Yes, precisely. I made a conscious design decision to keep the changes to a minimum and maintain complete compatibility. That is the reason why
config_files
is kept even though it doesn't make much sense in this new setup. Instead ofconfig_files
we should have something likeconfig_origin
. The same reason why the new solution isn't "abstract enough".Again the same applies to HasLocation. Location is deduced once and then passed on to the config readers. Just like environment name.