Change implementation from `prepend` to `UnboundMethod` #1

AlexWayfer · 2018-04-19T15:33:13Z

prepend pollutes Module#ancestors:

module Foo
end

class Bar
  prepend Foo
end

class Baz < Bar
  prepend Foo
end

Baz.ancestors # => [Foo, Baz, Foo, Bar, Object, Kernel, BasicObject]

Also, two modules with included Memery can broke the source of super-method.

And define_method with UnboundMethod is possible now.

coveralls · 2018-04-19T15:35:20Z

Pull Request Test Coverage Report for Build 15

11 of 11 (100.0%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 100.0%

Totals
Change from base Build 14:	0.0%
Covered Lines:	123
Relevant Lines:	123

💛 - Coveralls

JelF · 2018-04-19T15:46:43Z

lib/memery.rb

-      @_memery_module.module_eval do
-        define_method(method_name) do |*args, &block|
-          return super(*args, &block) if block
+      define_method(method_name) do |*args, &block|


It should be conviment way to obtain source of original method in pry or irb

Memery allows it with $ method_name -s in pry or method(:method_name).super_method.source in irb which is good

It should be conviment way to obtain source of original method in pry or irb

Then there should be specs for this.

I don't think that viewing the source code of memoized method from pry/irb is very important, but OK, it can be an argument.

Unfortunately, $ method_name -s doesn't work in Pry, but I hope that some day it will get fixed.
A.instance_method(:x).super_method.source technique does work, though, so we might want to just add that spec. I don't see much harm in having some extra ancestors.

irb(main):002:0> def foo irb(main):003:1> p :bar irb(main):004:1> end => :foo irb(main):005:0> method(:foo).source NoMethodError: undefined method `source' for #<Method: main.foo> from (irb):5 from /home/alex/.rbenv/versions/2.4.4/bin/irb:11:in `<main>'

What are you talking about?

A.instance_method(:x).super_method.source works everywhere in case you required method_source gem.

But that's just an example, the idea is, with current implementation it is possible to get the actual method's source code using Ruby, and with this implementation it becomes impossible.

OK. This MR is just offer, proof of concept, showcase of alternative way. If you think that getting source from Ruby (pry) is more important than a lot of Memery in ancestors in case of inheritance (and maybe some other unpleasant consequences, about which I don't remember or don't know) — you can just close this MR. And in the future, if some obvious shortcomings of the prepend approach will be revealed — you and others will know about another not bad (in my opinion) approach, and apply it, or create a fork with it.

Unfortunately, $ method_name -s doesn't work in Pry, but I hope that some day it will get fixed.

Just a ref to issue: pry/pry#1756

With this implementation you can show the source of original method by moving (stepping) into method from memery and calling $ original_method from method body.

Maybe, but you can't do method(:x).super_method.source as stated in Readme.

`prepend` pollutes `Module#ancestors`: ```ruby module Foo end class Bar prepend Foo end class Baz < Bar prepend Foo end Baz.ancestors # => [Foo, Baz, Foo, Bar, Object, Kernel, BasicObject] ```

tycooon · 2018-04-20T11:45:23Z

Cool, closing for now then. The ability to get the source code of the original method was kept in mind when I was building this gem, and it does not rely on pry functionality in any way (pry actually fails to show the supermethod source code in it's normal way when prepend is used).

AlexWayfer · 2018-04-20T11:49:19Z

pry actually fails to show the supermethod source code in it's normal way when prepend is used

It would be good to refer to issue in pry repo (existing or newly created).

AlexWayfer · 2019-09-19T13:24:29Z

We have a code in commercial project with ModuleOne.define_method(:foo, ModuleTwo.instance_method(:foo)), where ModuleTwo#foo is memoized. It raises the error (like no super method for 'foo') for current master implementation, but works in this branch.

Spec added.

Maybe you can review your decision.

tycooon · 2019-09-19T13:30:31Z

I often use the super_method technique now and I guess in this branch it's super hard to get the original method implementation.

AlexWayfer · 2019-09-19T13:53:02Z

OK. I've fixed our case with ModuleTwo.instance_method(:foo).super_method.

AlexWayfer · 2020-01-16T14:37:42Z

The bad case with show-source:

# .test.rb
# frozen_string_literal: true

require 'pry-byebug'
require_relative 'lib/memery'

module A
  include Memery

  memoize def foo
    'source'
  end
end

module B
  include Memery
  include A

  memoize def foo
    "Get this #{super}!"
  end
end

class C
  include B
end

c = C.new

binding.pry

puts c.foo

> ruby .test.rb

From: /home/alex/Projects/ruby/memery/.test.rb @ line 31 :

    26: 
    27: c = C.new
    28: 
    29: binding.pry
    30: 
 => 31: puts c.foo

pry: main > c.foo
=> "Get this source!"

pry: main > show-source c.foo

From: /home/alex/Projects/ruby/memery/lib/memery.rb @ line 55:
Owner: #<Module:0x000055d0fa6ab460>
Visibility: public
Number of lines: 17

define_method(method_name) do |*args, &block|
  if block || (condition && !instance_exec(&condition))
    return super(*args, &block)
  end

  key = "#{method_name}_#{mod_id}"

  store = (@_memery_memoized_values ||= {})[key] ||= {}

  if store.key?(args) && (ttl.nil? || Memery.monotonic_clock <= store[args][:time] + ttl)
    return store[args][:result]
  end

  result = super(*args)
  @_memery_memoized_values[key][args] = { result: result, time: Memery.monotonic_clock }
  result
end

pry: main > show-source -s c.foo

From: /home/alex/Projects/ruby/memery/lib/memery.rb @ line 55:
Owner: #<Module:0x000055d0fa6ab460>
Visibility: public
Number of lines: 17

define_method(method_name) do |*args, &block|
  if block || (condition && !instance_exec(&condition))
    return super(*args, &block)
  end

  key = "#{method_name}_#{mod_id}"

  store = (@_memery_memoized_values ||= {})[key] ||= {}

  if store.key?(args) && (ttl.nil? || Memery.monotonic_clock <= store[args][:time] + ttl)
    return store[args][:result]
  end

  result = super(*args)
  @_memery_memoized_values[key][args] = { result: result, time: Memery.monotonic_clock }
  result
end

pry: main > show-source -ss c.foo

From: /home/alex/Projects/ruby/memery/lib/memery.rb @ line 55:
Owner: #<Module:0x000055d0fa6abd98>
Visibility: public
Number of lines: 17

define_method(method_name) do |*args, &block|
  if block || (condition && !instance_exec(&condition))
    return super(*args, &block)
  end

  key = "#{method_name}_#{mod_id}"

  store = (@_memery_memoized_values ||= {})[key] ||= {}

  if store.key?(args) && (ttl.nil? || Memery.monotonic_clock <= store[args][:time] + ttl)
    return store[args][:result]
  end

  result = super(*args)
  @_memery_memoized_values[key][args] = { result: result, time: Memery.monotonic_clock }
  result
end

pry: main > show-source -sss c.foo

From: /home/alex/Projects/ruby/memery/lib/memery.rb @ line 55:
Owner: #<Module:0x000055d0fa6abd98>
Visibility: public
Number of lines: 17

define_method(method_name) do |*args, &block|
  if block || (condition && !instance_exec(&condition))
    return super(*args, &block)
  end

  key = "#{method_name}_#{mod_id}"

  store = (@_memery_memoized_values ||= {})[key] ||= {}

  if store.key?(args) && (ttl.nil? || Memery.monotonic_clock <= store[args][:time] + ttl)
    return store[args][:result]
  end

  result = super(*args)
  @_memery_memoized_values[key][args] = { result: result, time: Memery.monotonic_clock }
  result
end
pry: main > show-source -ssss c.foo
Error: Couldn't locate a definition for c.foo

AlexWayfer · 2020-01-16T15:10:23Z

I've updated the branch if you'll be interested.

(but PR is not updated somewhy)

Also, with the example (.test.rb) above I can view sources of methods in this way:

pry: main > c.method(:foo).owner.memoized_methods[:foo].source
=> "  memoize def foo\n    \"Get this \#{super}!\"\n  end\n"
pry: main > c.method(:foo).super_method.owner.memoized_methods[:foo].source
=> "  memoize def foo\n    'source'\n  end\n"

AlexWayfer · 2020-04-24T11:29:24Z

And another issue:

Using `any_instance` to stub a method (live?) that has been defined on a prepended module (#<Module:0x000055b316a1bc28>) is not supported.

I can create a literally issue, but this PR is resolving them all.

JelF suggested changes Apr 19, 2018

View reviewed changes

Change implementation from prepend to UnboundMethod

30cbebb

`prepend` pollutes `Module#ancestors`: ```ruby module Foo end class Bar prepend Foo end class Baz < Bar prepend Foo end Baz.ancestors # => [Foo, Baz, Foo, Bar, Object, Kernel, BasicObject] ```

AlexWayfer force-pushed the refactor_to_unbound_method branch from 0d62f66 to 30cbebb Compare April 20, 2018 11:22

tycooon closed this Apr 20, 2018

AlexWayfer mentioned this pull request Aug 13, 2020

debug with memery #32

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change implementation from `prepend` to `UnboundMethod` #1

Change implementation from `prepend` to `UnboundMethod` #1

AlexWayfer commented Apr 19, 2018 •

edited

Loading

coveralls commented Apr 19, 2018 •

edited

Loading

JelF Apr 19, 2018

AlexWayfer Apr 19, 2018

AlexWayfer Apr 19, 2018

tycooon Apr 19, 2018

AlexWayfer Apr 19, 2018 •

edited

Loading

tycooon Apr 20, 2018

AlexWayfer Apr 20, 2018

AlexWayfer Sep 19, 2019

AlexWayfer Sep 19, 2019

tycooon Sep 19, 2019

tycooon commented Apr 20, 2018

AlexWayfer commented Apr 20, 2018

AlexWayfer commented Sep 19, 2019 •

edited

Loading

tycooon commented Sep 19, 2019

AlexWayfer commented Sep 19, 2019

AlexWayfer commented Jan 16, 2020 •

edited

Loading

AlexWayfer commented Jan 16, 2020 •

edited

Loading

AlexWayfer commented Apr 24, 2020

Change implementation from prepend to UnboundMethod #1

Change implementation from prepend to UnboundMethod #1

Conversation

AlexWayfer commented Apr 19, 2018 • edited Loading

coveralls commented Apr 19, 2018 • edited Loading

Pull Request Test Coverage Report for Build 15

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexWayfer Apr 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tycooon commented Apr 20, 2018

AlexWayfer commented Apr 20, 2018

AlexWayfer commented Sep 19, 2019 • edited Loading

tycooon commented Sep 19, 2019

AlexWayfer commented Sep 19, 2019

AlexWayfer commented Jan 16, 2020 • edited Loading

AlexWayfer commented Jan 16, 2020 • edited Loading

AlexWayfer commented Apr 24, 2020

Change implementation from `prepend` to `UnboundMethod` #1

Change implementation from `prepend` to `UnboundMethod` #1

AlexWayfer commented Apr 19, 2018 •

edited

Loading

coveralls commented Apr 19, 2018 •

edited

Loading

AlexWayfer Apr 19, 2018 •

edited

Loading

AlexWayfer commented Sep 19, 2019 •

edited

Loading

AlexWayfer commented Jan 16, 2020 •

edited

Loading

AlexWayfer commented Jan 16, 2020 •

edited

Loading