Giter VIP home page Giter VIP logo

Comments (9)

vovanmozg avatar vovanmozg commented on June 10, 2024

I have two suggestions about interface of the feature functionality:

Convention Over Configuration Approach:
Enforce a standardized file organization for configurations. For example, files should be loaded in the following sequence:

  1. Basic Configuration: Always loaded first (settings.yml).
  2. General Configuration Files: Placed in a default directory and applied across all environments (settings/default/separated_config.yml, settings/default/subdir/another_separated_config.yml).
  3. Environment-Specific Configuration: Overrides for specific environments (settings/production.yml).
  4. Overloading Configuration Files: Allows further customization within specific environments (settings/production/separated_config.yml, settings/production/subdir/another_separated_config.yml).
    Potential Risk: If users already have some files in settings/default, they might encounter issues after updating the configuration gem. The new version could attempt to process these files, leading to unexpected behavior.

Extending Configuration:
Provide a mechanism to modify the order of configuration files loaded by the Config.process_settings_files method:

Config.setup do |config|
  config.process_settings_files do |settings_files|
    # Add files to the beginning of the list
    settings_files.unshift('config/settings/default/separated_config.yml')
    # Append files to the end of the list
    settings_files << 'config/settings/default/another/separated_config.yml'
    # Or redefine settings_files
    settings_files = ['some_config.yml']
    settings_files
  end
end

This approach offers flexibility in configuring the loading order, ensuring users can prioritize their configuration files as needed.

I can try to prepare PR

from config.

cjlarose avatar cjlarose commented on June 10, 2024

I think you're hitting on all of things that are important to consider

  • Rails users, broadly, are accustomed to convention-over-configuration. The default Railtie already loads config/development.yml for example if loading Rails in development mode. Conversely, when loaded outside of Rails, users are expected to be explicit about which files are loaded (they must call Config.load_and_set_settings themselves)
  • Ideally, changes would be made an a backward-compatible way. Loading additional paths by default in a version bump is probably fine, but removing them should be avoided

I think the idea of being able to have control over the order over which files are loaded, and I think the suggested API for config.process_settings_files would make sense for Rails projects, where there is essentially a "default" set of settings files. For non-Rails use cases, though, it doesn't really make sense to be passed the "default" set of settings files since there really isn't one. I think I'd be more inclined to have something where the user just gives the explicit list of files (either as an Array or as a block that yields an Array)

Config.setup do |config|
  config.default_settings_sources = [
    "config/settings.yaml"
  ]
end

The default value for the new config.default_settings_sources setting can just be empty. Or if you're in a Rails project, it should be set to basically Config.setting_files(::Rails.root.join('config'), ::Rails.env). If they want to express their list of settings files by manipulating the default list of files programmatically, they can by calling Config.setting_files explicitly.

Config.setup do |config|
  config.default_settings_sources = do
    settings_files = Config.setting_files(::Rails.root.join('config'), ::Rails.env)
    settings_files.unshift('config/settings/default/separated_config.yml')
    settings_files << 'config/settings/default/another/separated_config.yml'
  end
end

What do you think?

from config.

cjlarose avatar cjlarose commented on June 10, 2024

Thinking about it a bit more, I guess both approaches depart from what I think is the original design of the gem:

  1. Config.setup is used configure the behavior of the gem (without actually specifying the literal list of files).
  2. Config.load_and_set_settings actually takes in the list of files. For non-Rails projects, this must be called explicitly. In Rails projects, this is called automatically in the Railtie. In fact, calling it from config/initializers/config.rb will actually be ignored.

This distinction is interesting to me because Config.setup is used to specify things that are not related to the list of files to load like how merging settings hashes should behave and which constant to use (by default, Settings), but also things that are definitely related to where settings should come from (e.g. use_env and, more recently, file_name and dir_name). file_name and dir_name, I think exist largely because in a Rails project, the user doesn't have a lot of control over the actual list of settings files. Those two options only really have an effect when calling Config.setting_files.

I'm not 100% bought-in that this present distinction between config and data (the actual list of files) is the perfect API, but I also want to respect the precedent and I think being able to specify the list of settings files in both Config.setup and Config.load_and_set_settings would be confusing.

Therefore, I'm thinking the most straightforward solution to this problem is to just make it so that if a Rails user calls Config.load_and_set_settings in config/initializers/config.rb, then the Railtie shouldn't also call the same function (and thereby essentially overwrite the settings loaded in the initializer). The API for Config.load_and_set_settings already gives users the ultimate flexibility in what files are loaded. The actual problem, I think, is just that the Railtie clobbers any use of that method by doing its own Config.load_and_set_settings.

So for Rails uses that are completely bought in to the default list of settings files, they don't have to call Config.load_and_set_settings themselves. But for Rails users that actually do want to customize the list of files, they are welcome to by calling Config.load_and_set_settings in the initializer with whatever list of files they like.

from config.

vovanmozg avatar vovanmozg commented on June 10, 2024

I understand your position and agree that there is a reason to stick to the original plan.

I like the idea of having Railtie not call Config.load_and_set_settings if the user has already called it before.

I tried to think about how this could break backward compatibility, but I couldn't find any issues. reload_from_files, which is called at runtime but later than the initial initialization, so it seems there shouldn't be any problems with it. If users, for some reason, called Config.load_and_set_settings earlier, Railtie still did not take this into account.

Then the project's config might look like this:

# app/initializers/config.rb in a Rails project
Config.setup do |config|
  Config.load_and_set_settings(file_list)
end

And if the configuration is already defined (load_and_set_settings has already been loaded), then we do not perform the reloading.

class Railtie < ::Rails::Railtie  
  def preload  
    ...
    return if Object.const_defined?(Config.const_name)  
  
    # Parse the settings before any of the initializers  
    Config.load_and_set_settings(  
      Config.setting_files(::Rails.root.join('config'), ::Rails.env)  
    )
    ...

from config.

cjlarose avatar cjlarose commented on June 10, 2024

Yeah, that's perfect. Would you be interested in opening a PR?

from config.

vovanmozg avatar vovanmozg commented on June 10, 2024

Yes, I will try

from config.

vovanmozg avatar vovanmozg commented on June 10, 2024

#353

I'm not found a way to test it yet.

I need to first execute the Config.Setup code, and after that it should execute Config::Integrations::Rails::Railtie.preload

But all specs runs after Config::Integrations::Rails::Railtie.preload. So we can not test it with standard bundle exec appraisal rails...

from config.

cjlarose avatar cjlarose commented on June 10, 2024

Yeah, there's not a good existing test pattern to use for this case. There are a few Rails apps in spec/app, but currently we just run the tests after executing require_relative "app/#{app_name}/config/environment". That's useful for running the complete test suite under the environment of a particular Ruby/Rails version combination, but indeed this "pollutes" the test environment if we're trying to test something about the Rails boot sequence itself.

Without completely reorganizing the test suite, the most straightforward approach I see is to test the Railtie behavior is something like

  1. Write a new file to Rails.root.join('config', 'initializers', 'config.rb')
  2. Write any necessary YAML files e.g. Rails.root.join('config', 'settings.yml')
  3. Make assertions on Settings based by interacting with the current Rails app via the CLI (e.g. ./bin/rails runner 'puts Settings.expected_key'). This way, the Rails app will be loaded in a realistic way (the Railtie should run, then the initializers as normal) and it isn't polluted by the fact that the test process itself has already loaded the Rails app.
  4. Delete the temporary files

We can rinse and repeat this pattern for a few tests

  1. Without any config.rb, the default setting files are loaded (anything in config/settings.yml for example)
  2. With a config.rb, but without any use of Config.load_and_set_settings, the desired settings apply (for example file_name or dir_name), and the same default setting files are still loaded (if file_name is set to config, then config.yml is loaded)
  3. With a config.rb that calls Config.load_and_set_settings, the desired settings apply and only the specified files are loaded (and not any other yml files like config/settings.yml)

from config.

vovanmozg avatar vovanmozg commented on June 10, 2024

Thank you for great advice! I added tests

from config.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.