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

Refactor to use memoizable #24

Merged
merged 7 commits into from Dec 14, 2013
Merged

Refactor to use memoizable #24

merged 7 commits into from Dec 14, 2013

Conversation

dkubb
Copy link
Owner

@dkubb dkubb commented Nov 21, 2013

This branch refactors adamantium to use memoizable, which are the memoization methods extracted from adamantium, generalized and usable for all kinds of objects.

@dkubb
Copy link
Owner Author

dkubb commented Nov 21, 2013

@sferik it was relatively easy to replace the memoization code in adamantium with memoizable. IMHO that means the extraction was successful :)

@@ -4,6 +4,8 @@ source 'https://rubygems.org'

gemspec

gem 'memoizable', git: 'https://github.com/dkubb/memoizable.git'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary? If so, should we push another gem release? It appears that latest memoizable gem (v0.2.0) has diverged from master by a few commits.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's necessary because of a few changes I made in memoizable; nothing that breaks the interface though.

I'll probably release memoizable 0.3.0 once I test all this properly with some other dependent gems.

@sferik
Copy link

sferik commented Nov 21, 2013

👍 Nice work on this! 😄

else :public
end
memoized_methods[method_name] = MethodBuilder
.new(self, method_name, freezer).call
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dkubb I already was thinking hard about creating a MethodObject mixin that would support transforming:

MethodBuilder.new(self, method_name, freezer).call into MethodBuilder.call(self, method_name_freezer)

Creating an instance and delegating to #call:

def self.call(*arguments)
  new(*arguments).call
end

Would DRY up a lot of places in my code.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mbj do you find yourself doing this kind of thing in your gems that dynamically add methods? I really like this approach and will probably even look at doing it in equalizer.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mbj @dkubb i do that kind of thing very often! i haven't yet thought about extracting something generic from it, but if a nice api crops up, i already know that i'd use it in a ton of places, public and private code.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usage of such a method_object gem would be as simple as:

class MethodBuilder
  include MethodObject, Concord.new(:object, :method_name, :freezer)

  def call
     calculate_something_based_on_instance_state
  end
end

MethodBuilder.call(self, method_name, freezer)

@dkubb
Copy link
Owner Author

dkubb commented Dec 2, 2013

@mbj I'm almost ready to merge this in, but the blocker is that mutant doesn't seem to be able to zombify memoizable or thread_safe/cache. I tried added them explicitly to mutant.rb, like you hinted in irc, but it didn't seem to make a difference.

I'd really like to merge this in, but as it is now it seems to break mutant with every gem that depends on adamantium, which is basically all of our gems. Once this blocker is resolved, it will have a domino effect and allow me to release a half dozen other gems that are waiting on this extraction, including a new version of memoizable.

Is there any way you can find some time this week to look at this?

@mbj
Copy link
Collaborator

mbj commented Dec 2, 2013

@dkubb Yeah, I'll look into it after todays commercial activities. PLS ping me on IRC to be sure ;)

@dkubb
Copy link
Owner Author

dkubb commented Dec 2, 2013

@mbj since this is blocking other gem releases, maybe I should just merge this into master now?

Zombiefication in mutant will be broken however, but that may be a better alternative than blocking a ton of gems.

@dkubb
Copy link
Owner Author

dkubb commented Dec 2, 2013

@mbj btw, the only change I made to mutant after this extraction is:

diff --git a/lib/mutant/matcher/method/instance.rb b/lib/mutant/matcher/method/instance.rb
index be2aa9a..1b965fa 100644
--- a/lib/mutant/matcher/method/instance.rb
+++ b/lib/mutant/matcher/method/instance.rb
@@ -73,7 +73,7 @@ module Mutant
           # @api private
           #
           def source_location
-            scope.original_instance_method(method.name).source_location
+            scope.unmemoized_instance_method(method.name).source_location
           end

         end # Memoized

EDIT: the reason I changed this that I didn't like the term "original". It could be anything. With unmemoized it's much more clear what you're asking for, and that it's related to the memoizable gem.

@mbj
Copy link
Collaborator

mbj commented Dec 3, 2013

@dkubb Yeah. Do so. I'll fix once I have the time. Sorry for yesterday. To busy :(

dkubb added a commit that referenced this pull request Dec 14, 2013
@dkubb dkubb merged commit ca495bc into master Dec 14, 2013
@dkubb dkubb deleted the refactor-memoizable branch December 14, 2013 07:17
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.

None yet

4 participants