delegate_missing_to :instance fails when config_attr is an existing class method.
jhirn opened this issue · 1 comments
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 addsdelegate <: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
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?