-
-
Notifications
You must be signed in to change notification settings - Fork 235
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
base: master
Are you sure you want to change the base?
Conversation
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 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. |
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. |
@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. |
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 Remember that a 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 Of course, the new Let me know if that makes sense, and more importantly, if it'd address your original use case. |
@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. |
This PR adds functionality for optional namespaces when creating a new
Config::Sources::YAMLSource
source or usingSettings.add_source!
per discussion in #288.The functionality works like this:
Given a YAML file like this:
The code works like this:
Tests have been written and run to cover both cases based entirely on the existing
basic yml file
test case, calledbasic yml file with single namespace
andbasic 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.