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

Cache built-in query methods of immutable object #25

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dkubb
Copy link
Owner

@dkubb dkubb commented Dec 16, 2013

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]

@mbj
Copy link
Collaborator

mbj commented Dec 16, 2013

@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 #hash, #inspect etc?

@dkubb
Copy link
Owner Author

dkubb commented Dec 16, 2013

@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 #hash (or #inspect or #to_s) then they'll get tons of warnings which are meaningless.

@elskwid
Copy link
Contributor

elskwid commented Dec 16, 2013

@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.

@mbj
Copy link
Collaborator

mbj commented Dec 16, 2013

@dkubb IMHO: I'd be okay to issue a warning. If someone dislikes these warnings he can define their own #hash and friends, than import Adamantium or dont use this library. I'd generally favor doing what fulfills my needs, and than focus on imaginary users. Avoiding doing decisions and creating to much options just limits effectiveness of the lib and development speed: "Library authors should provide solutions, not more options.".

My typical code looks like this:

class Foo
  include Adamantium, Concord.new(:bar, :baz)
end

Module#include applies includes in reverse order, concord uses equalizer to set up #hash and friends. So adamantium with auto memoization of #hash and friends would fit my style perfectly.

@dkubb
Copy link
Owner Author

dkubb commented Dec 16, 2013

@mbj what about using method_added to memoize #to_s, #to_hash and #inspect when they are added?

@mbj
Copy link
Collaborator

mbj commented Dec 16, 2013

@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.

@mbj
Copy link
Collaborator

mbj commented Dec 16, 2013

@dkubb Maybe you should NOT memoize #hash and friends by default. Only if a method missing was detected!

@dkubb
Copy link
Owner Author

dkubb commented Dec 16, 2013

@mbj method_missing won't kick in because they'll inherit those methods from Object.

@mbj
Copy link
Collaborator

mbj commented Dec 16, 2013

@dkubb I meant method_added. Sorry obviousely NOT method missing ;)

@orend
Copy link

orend commented May 19, 2014

This PR needs to be rebased; bundle install fails because of old gem dependencies.

@dkubb
Copy link
Owner Author

dkubb commented Aug 11, 2014

@orend I rebased this against master so it should be usable.

@dkubb
Copy link
Owner Author

dkubb commented Aug 11, 2014

My latest commit adds a #method_added hook that will memoize a method when it is added.

However, it loses the ability to memoize the built-in #to_s and #inspect methods. I couldn't make them both work because you can't re-memoize a method twice in the same class.

What I think may need to happen is a change in memoize itself where you can memoize a specific method only once, but if the method is overridden you can memoize it again. Once this change is done, then the built-in #to_s and #inspect methods can be automatically memoized again.

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 this pull request may close these issues.

Memoize methods that depend on instance state automatically
4 participants