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

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

Closed
jhirn opened this issue Mar 6, 2023 · 1 comment
Assignees

Comments

@jhirn
Copy link

jhirn commented Mar 6, 2023

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
Copy link
Owner

palkan commented Mar 6, 2023

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?

@palkan palkan closed this as completed in 8c6ac5d May 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants