palkan / anyway_config

Configuration library for Ruby gems and applications
MIT License
778 stars 52 forks source link

Add loader_options, use to add ejson_namespace nil support #126

Closed bessey closed 1 year ago

bessey commented 1 year ago

What is the purpose of this pull request?

Ensures that EJSON, like the ENV and Doppler loaders, supports not utilising the namespaced configuration feature.

Happy to follow up with tests and docs if you think this is reasonable!

Is there anything you'd like reviewers to focus on?

~To keep the API surface small I have used the env_prefix keyword, even though its not really a prefix in this context (JSON nested keys). I figure consistency is preferable.~

Checklist

palkan commented 1 year ago

Thanks for the proposal!

Agree, it makes. Though I'm not sure env_prefix is the right way to handle this.

I think, we should introduce an API to pass custom options to loaders. Smth like:

class MyConfig < Anyway::Config
  loader_options ejson_prefix: "" 
end
bessey commented 1 year ago

Hey @palkan, yeah I prefer that, certainly more modular for those adding their own loaders.

I went with ejson_namespace as I felt "namespace" more accurately describes the concept in JSON. "prefix" makes sense when its string concatenation, but here we are nesting JSON blobs within X. X = a namespace.

Introduced some low level tests but I wasn't sure how best to write tests for Config since right now only the EJSON loader uses this functionality.

I also used nil to signify "no namespace" but I think perhaps false would be a better option actually. I know prefix uses "" but again, that doesn't seem appropriate in this context to me. Its a small change though so if that consistency is preferred, its easily done.

If you want to take this PR over at this point, feel free to make your own changes.

bessey commented 1 year ago

All changes made and hopefully CI passes now (I don't have JRuby to test against so somewhat coding blind there)

palkan commented 1 year ago

Thanks! I'll take care of linting issues.