config icon indicating copy to clipboard operation
config copied to clipboard

Add namespaces to YAML sources per #288

Open wwboynton opened this issue 4 years ago • 7 comments

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 avatar Mar 06 '21 22:03 wwboynton

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 avatar Mar 07 '21 03:03 wwboynton

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.

wwboynton avatar Mar 07 '21 04:03 wwboynton

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

pkuczynski avatar Mar 07 '21 15:03 pkuczynski

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

Yep! I can take a look this week.

cjlarose avatar Mar 08 '21 18:03 cjlarose

@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.

wwboynton avatar Mar 11 '21 08:03 wwboynton

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.

cjlarose avatar Mar 16 '21 05:03 cjlarose

@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.

wwboynton avatar Jun 05 '22 04:06 wwboynton