Skip to content
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

Allow a custom environment variable prefix to include the environment variable separator #177

Merged
merged 5 commits into from
Aug 7, 2017
Merged

Allow a custom environment variable prefix to include the environment variable separator #177

merged 5 commits into from
Aug 7, 2017

Conversation

rdodson41
Copy link
Contributor

Currently, given the following configuration:

Config.setup do |config|
  config.use_env = true
  config.env_prefix = 'MY_CONFIG'
  config.env_separator = '_'
  config.env_converter = :downcase
  config.env_parse_values = true
end

an environment variable, such as MY_CONFIG_USERNAME=rdodson41, will be split into ['MY', 'CONFIG', 'USERNAME'], and, therefore will fail to match against the prefix MY_CONFIG.

This pull request will allow a custom environment variable prefix to include the environment variable separator by splitting the prefix on the separator and, only then, comparing the beginning of the split variable name to the split prefix. As a result, ['MY', 'CONFIG', 'USERNAME'] will successfully match against the prefix MY_CONFIG, which would be split into ['MY', 'CONFIG'].

I don't think that this is a breaking change, as currently, given such a configuration, no environment variables would be merged into the configuration, but I am not completely sure.

@pkuczynski
Copy link
Member

Code climate complain about Config::Options#split doesn't depend on instance state (maybe move it to another class?). At the same time Config.env_prefix and Config.const_name should be already strings, so no point to convert them.

Could you please inline split method when appropriate?

Other than that looks good! :)

@rdodson41
Copy link
Contributor Author

rdodson41 commented Aug 7, 2017

@pkuczynski Thanks for the feedback; I reverted the commit in question. Should be good now.

@rdodson41
Copy link
Contributor Author

@pkuczynski I fixed the other two Code Climate issues; the single remaining issue does not originate in this pull request, so I left it unchanged.

With regard to (Config.env_prefix || Config.const_name).to_s.split(separator), I agree that .to_s should not be necessary, as both of these configuration variables should be strings, but I retained .to_s so that this method does not raise an error if, for some reason, a user configures one or both variables as symbols.

@pkuczynski
Copy link
Member

Looks good! Thanks!

@pkuczynski pkuczynski merged commit 3131b54 into rubyconfig:master Aug 7, 2017
@rdodson41 rdodson41 deleted the allow-env-prefix-with-separator branch August 7, 2017 17:28
@pkuczynski pkuczynski added this to the 1.5.0 milestone Oct 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants