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

Add namespaces to YAML sources per #288 #301

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

wwboynton
Copy link

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
Copy link
Author

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
Copy link
Author

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
Copy link
Member

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

@cjlarose
Copy link
Member

cjlarose commented Mar 8, 2021

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

Yep! I can take a look this week.

@wwboynton
Copy link
Author

@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
Copy link
Member

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
Copy link
Author

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

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.

None yet

3 participants