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

Optional namespace for add_source! #288

Open
wwboynton opened this issue Nov 24, 2020 · 14 comments
Open

Optional namespace for add_source! #288

wwboynton opened this issue Nov 24, 2020 · 14 comments

Comments

@wwboynton
Copy link

wwboynton commented Nov 24, 2020

It would be nice to be able to invoke Settings.add_source! with an optional parent prefix such that new sources could be loaded in programmatically into different levels of the Settings structure. This would allow for multiple YAML files of similar function to be grouped under the same parent without having to specify that namespace in every single YAML file.

In my case, I have a lot of user-defined YAML files together in a directory which are split up for organization/legibility, but which cohabitate in the same parent node of the Settings object when all loaded together. It's confusing to ask my users to specify a few levels of parent nodes at the top of every single file in the directory just so that the Settings object will be correctly organized when it gets to my code. It would be nice to write code which will load these YAML files into the Settings object in a way that mimics their directory structure on the filesystem.

Modifying the yaml_source.rb source to accommodate this might look something like this:

# config/lib/config/sources/yaml_source.rb 

module Config
  module Sources
    class YAMLSource
      attr_accessor :path

      def initialize(path, namespace = nil)  # New variable here
        @path = path.to_s
        @namespace = namespace 
      end

      # returns a config hash from the YML file
      def load
        result = YAML.load(ERB.new(IO.read(@path)).result) if @path and File.exist?(@path)

        if [email protected]? && result        # New logic here
          result = @namespace.reverse.inject(result) { |a, n| { n => a } }
        end
        result || {}
        rescue Psych::SyntaxError => e
      
        # ...etc

Running that like so:

puts "Without Namespace!"
y = Config::Sources::YAMLSource.new("/Users/userguy/repos/scratch/example.yml")
pp y.load

puts "With Namespace!"
y = Config::Sources::YAMLSource.new("/Users/userguy/repos/scratch/example.yml", ['new-level'])
pp y.load

...produces output like this:

Without Namespace!
{"foo"=>["bar", "baz", {"chungus"=>["Bungus"]}]}
With Namespace!
{"new-level"=>{"foo"=>["bar", "baz", {"chungus"=>["Bungus"]}]}}

If you'd like me to take a crack at doing this and submitting a PR, let me know and I'm happy to do it. Maybe I am just very tired and completely missed an existing way to do this in the documentation, or am overthinking this and forgetting something fundamental about YAML that will solve my problem. It has been known to happen, in which case please close this issue and accept my apologies for wasting your time!

@pkuczynski
Copy link
Member

Adding sources at runtime should be rather done this way: https://github.com/rubyconfig/config#adding-sources-at-runtime

However config has been designed to offer automatic env aware load, rather than manually stitching few files. I am not entirely sure if what you propose does not go against this concept. @cjlarose @Fryguy @rdubya what you think?

@Fryguy
Copy link
Member

Fryguy commented Nov 24, 2020

I think this feature is kind of cool. I could see people using it for dev/test/prod type breakdowns as well, where they want all 3 to appear in the final payload (as opposed to only one being merged based on env). If this feature is added, it just has to not conflict with the existing loading mechanisms.

@Fryguy
Copy link
Member

Fryguy commented Nov 24, 2020

I don't understand why there's an Array for the namespace, unless you're expecting it to nest multiple levels or something?

-Config::Sources::YAMLSource.new("/Users/userguy/repos/scratch/example.yml", ['new-level'])
+Config::Sources::YAMLSource.new("/Users/userguy/repos/scratch/example.yml", 'new-level')

@wwboynton
Copy link
Author

Yep, that's precisely it. Providing multiple items to the array nests the result progressively deeper @Fryguy . That's what that whole reverse-inject combo takes care of doing.

@wwboynton
Copy link
Author

@Fryguy -- With that said, you're not wrong that it's a little unnatural, lol. It certainly wouldn't be all that hard to slip in the whole if namespace.is_a?(Array); foo; else; bar thing to make the assignment of one-level namespaces a little more natural. I actually did that originally but took it out because I didn't want to provide an example of the idea that was too fluffy here, but I'm in favor of it.

@cjlarose
Copy link
Member

cjlarose commented Nov 24, 2020

It's already possible to pass an arbitrary Hash to Settings.add_source!, so the new feature suggestion seems redundant, unless I'm misunderstanding it. Seems like you can just add the new top-level keys using that mechanism.

nested_settings = Config::Sources::YAMLSource.new("/Users/userguy/repos/scratch/example.yml")
Settings.add_source!({ new_level: nested_settings.load })

@Fryguy
Copy link
Member

Fryguy commented Nov 24, 2020

@cjlarose That's a good point...I completely forgot that add_source can take arbitrary hashes, so yes I agree with you.

EDIT: However, will reload! work with that?

@pkuczynski
Copy link
Member

Reload would not work with the proposed namespace either I guess?

@Fryguy
Copy link
Member

Fryguy commented Nov 24, 2020

In the proposal the namespace is part of the source, so it could be reloaded.

@wwboynton
Copy link
Author

@cjlarose -- I did try that exact thing, but I remember experiencing some undesirable behaviour on reloads, as chunks of the configuration would just disappear. That's why I went to the source to make my modification so that this would persist through reloads. If you wanted to modify the behaviour of hash sources and nested objects inside to achieve the same effect, I wouldn't have a strong opinion about it (not really my place to have a strong opinion about a project you all maintain, anyway) but it seems from my uneducated vantage point like it would be more complicated to plumb in reliably.

@pkuczynski
Copy link
Member

If others agree, I am fine with accepting a PR with this feature...

@cjlarose
Copy link
Member

@wwboynton Makes sense. reload depends on calling load for all of the sources and since the Hash is unmodified, the workaround I suggested doesn't work for with reloading. The suggestion for adding a new namespace for the YAML source seems good!

@pkuczynski
Copy link
Member

@wwboynton would you still like to fire up PR for this?

@wwboynton
Copy link
Author

Hey, all. I'm sorry! When I wrote this issue, I was unemployed. Less than a week after, I ended up with a new job and therefore zero free time. But this weekend I was working on a project, used this gem, encountered this same thing again, and instantly remembered this whole conversation, so I've taken a moment and cracked open a fork with a PR coming soon.

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, I don't pretend to know my way all around this codebase and I'm happy to defer to your judgement, I just know this is functionality that I need and others might benefit from it, too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants