search

elixir-inspector / ua_inspector

User agent parser library
Apache License 2.0
125 stars 23 forks source link

adds the possibility to get db path from system env #5

Closed kaarebs closed 8 years ago

kaarebs commented 8 years ago

In a phoenix framework I use something like {:system, "ENV_VAR"} in my config.exs .. I would like to set application DB path from ua_inspector this env_var

mneudert commented 8 years ago

Can you give me a bit more details on the issue that makes you want to go that way?

I am not against this change. But once I include it here I probably need to do it in some other libraries as well, just for the sake of consistency. So at least I would like to put some effort into finding a solution that solves the shortcomings of the config files and the way they are evaluated locally and in packaged releases.

For example it could be a way to rework it like this:

System.get_env("UA_INSPECTOR_PATH") || Application.get_env(:ua_inspector, :database_path, nil)

This way system environment would take precedence over the application environment. That of course would only make sense if the latter is suitable most of the time except for some "edge cases".

With the recent elixir-lang mailing list discussion around that topic the generic system approach might be the one to got to for now. But that would still mean there has to be a test and some documentation ;)

kaarebs commented 8 years ago

Its fine .. This also solves my problem :)

mneudert commented 8 years ago

After trying some things out the "pre-defined os variable" solution breaks completely down once more than one value is needed. Nice thought but completely unusable :see_no_evil:

I just pushed a branch with your solution including a test and some documentation update to system-config.

Could you take a look and ensure it is working/documented you would expect?

mneudert commented 8 years ago

Glad to hear it works. I will leave the branch available until a new version is released. That will hopefully be next week once I have a proper "otp reload logic" working and in place.

And thanks for the suggestion/patch :+1: