-
Notifications
You must be signed in to change notification settings - Fork 13
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
Cache built-in query methods of immutable object #25
base: master
Are you sure you want to change the base?
Conversation
@dkubb I think the intent of this change is okay. But I fear we run into ordering issues with equalizer, concord, anima includes. Can we guard against overriding the memoized |
@mbj I wonder if a method_added hook could be used to warn when a memoized method is being overridden in the same class/module it's defined within? Or it could just re-memoize the method. One downside to warning is if someone defines their own |
@mbj, @dkubb - now that these gems are being combined in interesting ways seems to be a good time to talk about the ordering and needed methods. Ideally they don't need to be done in any order, right? The ordering gets increasingly complicated as we add more and more gems. It's ironic to be worried about overridden methods given some recent rants about ActiveSupport. I wish I had more to add to actually help. Mostly just echoing the concern and offering to help if I can. |
@dkubb IMHO: I'd be okay to issue a warning. If someone dislikes these warnings he can define their own My typical code looks like this: class Foo
include Adamantium, Concord.new(:bar, :baz)
end
|
@mbj what about using |
@dkubb Yeah. This will allow equalizer and similar libs to work without ANY knowlege about adamantium +1. I think this is the way to go. |
@dkubb Maybe you should NOT memoize |
@mbj method_missing won't kick in because they'll inherit those methods from |
@dkubb I meant |
This PR needs to be rebased; |
@orend I rebased this against master so it should be usable. |
My latest commit adds a However, it loses the ability to memoize the built-in What I think may need to happen is a change in |
This branch memoizes the
#hash
,#to_s
and#inspect
methods automatically since they are based on the instance state, and cannot change since the instance state is immutable.[Fix #9]