palkan/anyway_config

delegate_missing_to :instance fails when config_attr is an existing class method.

jhirn opened this issue · 1 comments

jhirn commented

What did you do?

The generator and guide suggest using delegate_missing_to :instance as a convenience for singleton behavior. This is incredibly handy but I just got bitten by it a 2nd time and I'm nervous about others incurring this problem down the line. Given this example, MyConfig.current_env and MyConfig.name will not resolve properly.

class ApplicationConfig < Anyway::Config
  class << self
    # Make it possible to access a singleton config instance
    delegate_missing_to :instance

    private

    def instance
      @instance ||= new
    end
end

class MyConfig < ApplicationConfig
    attr_config name: "my_config_name", 
                        current_env: "development",
                        etc.... # any instance method of ApplicationConfig
end

# Actual
MyConfig.name #=> ApplicationConfig
MyConfig.current_environment #=> Rails.env

# Expected
MyConfig.name #=> "my_config_name"
MyConfig.current_environment #=> "development"
(Or raise exception for shadowing if superclass has method defined) 

I understand the underlying issue, but it's a bit insidious in that it doesn't warn or fail eagerly when there is a collision and the attr is shadowed. I've renamed the attrs which solves the problem, but the delegate_missing_to in general seems risky. I'm not sure what the universal solution to this may be. I would love to see either:

  • attr_config method creation adds delegate <:attr> to :instance for each attr rather than (or in addition to) the universal missing delgate in the super class. This could also cause problems for existing methods used internally (e.g. current_env
  • attr_config raises an error if the method is defined on the super class to warn the user of the collision.

Environment

Ruby Version: 3.1.2

Framework Version (Rails, whatever): Rails 7.0.4

Anyway Config Version: 2.3.1

palkan commented

This could also cause problems for existing methods used internally

Exactly. We will have to keep the list of Config class methods as reserved names then.

attr_config raises an error if the method is defined on the super class to warn the user of the collision.

We already do that for instance methods, but not for class methods. Sounds like a reasonable option, but potentially a breaking change (e.g., name must be prohibited).

I'd suggest starting with a note in the documentation regarding this caveat. Would you like to propose a PR?