rubyconfig / config

Easiest way to add multi-environment yaml settings to Rails, Sinatra, Padrino and other Ruby projects.
Other
2.11k stars 231 forks source link

Add namespaces to YAML sources per #288 #301

Open wwboynton opened 3 years ago

wwboynton commented 3 years ago

This PR adds functionality for optional namespaces when creating a new Config::Sources::YAMLSource source or using Settings.add_source! per discussion in #288.

The functionality works like this:

Given a YAML file like this:

foo:
  - bar
  - baz
  - chungus:
    - Bungus

The code works like this:

# No namespace, same as current functionality
y = Config::Sources::YAMLSource.new("/tmp/example.yml")
y.load
=> {"foo"=>["bar", "baz", {"chungus"=>["Bungus"]}]}

# Single namespace, array or string both fine
y = Config::Sources::YAMLSource.new("/tmp/example.yml", 'new-level')
y.load
=> {"new-level"=>{"foo"=>["bar", "baz", {"chungus"=>["Bungus"]}]}}

# Nested namespace, given as array of keys
y = Config::Sources::YAMLSource.new("/tmp/example.yml", ['new-level', 'level-2'])
y.load
=> {"new-level"=>"level-2"=>{"foo"=>["bar", "baz", {"chungus"=>["Bungus"]}]}}}

Tests have been written and run to cover both cases based entirely on the existing basic yml file test case, called basic yml file with single namespace and basic yml file with nested namespace respectively.

I'm happy to make any changes necessary for code style (i.e. keyword method argument, ternary as opposed to multi-line, whatever) or functionality in keeping with existing standards for this codebase of which I may be ignorant.

wwboynton commented 3 years ago

Deleting previous comments -- I'm an idiot, and the problem is my fault. In trying to figure out why 2.4 tests were failing, I didn't wipe my Gemfile.lock between ruby versions, causing me to stay locked on new and incompatible gem versions with an older ruby. I resolved the issue by being less of an idiot and looking at the logs, discovering that the root cause in the first place was that I'd left a pp line I had used for debugging in the spec, and 2.4 was before the time that pp didn't need to be required explicitly. So, in a way, running it through an old version of ruby helped catch my dumb embarrassing mistake.

Anyway, the tests appear to pass now in native ruby 2.4-2.7, so sorry to anyone who had to read my drivel in this thread earlier. Thanks again for your encouragement and I hope this code is useful now that it's received some more scrutiny and caffeine.

wwboynton commented 3 years ago

I guess I can't see why one wouldn't want the same functionality to be consistent for hash sources since they get used similarly, so for consistency I went ahead and replicated the work in the same way for the hash sources and tests.

pkuczynski commented 3 years ago

@cjlarose will you have some time to look into this one?

cjlarose commented 3 years ago

@cjlarose will you have some time to look into this one?

Yep! I can take a look this week.

wwboynton commented 3 years ago

@cjlarose will you have some time to look into this one?

Yep! I can take a look this week.

Awesome, thank you! Let me know if there's anything I should do differently and I'm happy to accommodate as time allows.

cjlarose commented 3 years ago

Hey @wwboynton sorry it took so long to get back to you! I ended up having a busy week.

Anyway, I love the enthusiasm and I like the overall idea. However, I had an idea that I wanted to run by you because I think it'll clean up the code a little and also make this feature available more broadly. For example, there's a new Sources::EnvSource that I added in #299 and users could have their own custom Sources, but I think the idea of prefixing stuff with a namespace could be useful for all kinds of Sources. Instead of modifying the existing Source implementations, I think we can use some good ol' fashioned composition here.

Remember that a source from the perspective of the config gem is just something that responds to .load and returns a Hash, whenever the user first initializes their settings or calls Settings.reload!. Using that, I think it's possible to write a new, higher-order source class that takes a namespace and another source to add the namespace to. So you could do something like

original_source = Config::Sources::YAMLSource.new "/path/to/source.yml"
namespaced_source = Config::Sources::NamespacedSource.new ['new-level', 'level-2'], original_source
Settings.add_source! namespaced_source

We could still offer the shorthand version with the optional namespace parameter Settings.add_source! "/path/to/source.yml", ['new-level', 'level-2'], but under the hood, it'd use the new NamespacedSource.

Of course, the new NamespacedSource just has to implement a load method itself, which would just invoke load on the original source and then add the namespace on top of it before returning it.

Let me know if that makes sense, and more importantly, if it'd address your original use case.

wwboynton commented 2 years ago

@cjlarose hilariously, I don't think I ever saw this reply on this PR. Sorry about that! I came back today to report a small bug I found and stumbled upon this by accident, lol.

I don't have time to build that solution out, but I can definitely confirm that it would have solved my original use-case and I think it definitely makes the library more flexible for handling multiple YAML files without a bunch of unnecessary levels of keys in the files themselves.