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

Remove delegate usage #493

Closed
fwininger opened this issue Jan 12, 2021 · 3 comments · Fixed by #522
Closed

Remove delegate usage #493

fwininger opened this issue Jan 12, 2021 · 3 comments · Fixed by #522

Comments

@fwininger
Copy link
Contributor

66f1d79#r45885127

delegate is a rails method not a native ruby method and the gem does'nt have rails dependency

https://apidock.com/rails/Module/delegate

this commit introduce a bug :

$ irb -I lib/ -r ice_cube
3.0.0 :001 > IceCube::Schedule.new
3.0.0 :002 > IceCube::Schedule.new.to_s
        7: from /ice_cube/lib/ice_cube/schedule.rb:316:in `block in to_s'
        5: from <internal:/.rvm/rubies/ruby-3.0.0/lib/ruby/3.0.0/rubygems/core_ext/kernel_require.rb>:85:in `require'
        4: from /ice_cube/lib/ice_cube/i18n.rb:3:in `<top (required)>'
        3: from /ice_cube/lib/ice_cube/i18n.rb:4:in `<module:IceCube>'
        2: from /ice_cube/lib/ice_cube/i18n.rb:8:in `<module:I18n>'
        1: from /ice_cube/lib/ice_cube/i18n.rb:9:in `singleton class'
NoMethodError (undefined method `delegate' for #<Class:IceCube::I18n>)
Did you mean?  DelegateClass
@pacso
Copy link
Collaborator

pacso commented Oct 22, 2021

@seejohnrun - as we discussed ... should we make activesupport a dependency of the gem rather than rolling back these changes?

@fwininger
Copy link
Contributor Author

@pacso / @seejohnrun I think it's really sad to add active support as dependency for just the delegate method.

A better solution is to replace :

delegate method to: :other_class

with :

def method; other_class.method; end

@seejohnrun
Copy link
Collaborator

@fwininger Thanks - I think that's a reasonable approach here but I'm wondering if you could take a bit to mention why adding activesupport might be a bad idea? I've held off on adding the dependency for a long time, but it seems pretty compelling since in a lot of places we conditionally have branches for using and not using ActiveSupport. Overall, AS should be an okay dependency even for non-Rails apps to link in but wondering if we're missing something about the criticality of keeping the dependency away. Thank you!

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

Successfully merging a pull request may close this issue.

3 participants