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

&block is omited when method memoized #39

Open
JelF opened this issue Dec 17, 2015 · 12 comments · May be fixed by #43
Open

&block is omited when method memoized #39

JelF opened this issue Dec 17, 2015 · 12 comments · May be fixed by #43

Comments

@JelF
Copy link

JelF commented Dec 17, 2015

def foo(&block)
  @block = block
end
memoize :foo

foo { :bar }
@block # => nil

There should be either exception or no memoization in this case

@matthewrudy
Copy link
Owner

@JelF what's your actual use case?

By default memoist will treat this like any method with arguments, and try and memoize it based on the value of the block object

But I'm not sure what a real use of this is, and therefore don't know what the correct behaviour should be.

@JelF
Copy link
Author

JelF commented Dec 17, 2015

@matthewrudy i tried to memoize #fetch, delegated to some hash, and got unexpected KeyError in specs

expect(subject.fetch(Foo::Bar) { {} }).to eq({}) # => KeyError

@JelF
Copy link
Author

JelF commented Dec 17, 2015

By default memoist will treat this like any method with arguments, and try and memoize it based on the value of the block object

As far as i see, method defined in lib/memoist.rb does not accept &blocks (they are not catched by *args). So this is wrong, memoize just ignores any &blocks

@matthewrudy
Copy link
Owner

@JelF you're correct, sorry.

@matthewrudy
Copy link
Owner

But again, what's the real use case, something like this?

class Store
  def fetch(key)
    get(key) || set(key, yield)
  end
end

something like this?

@JelF
Copy link
Author

JelF commented Dec 17, 2015

@JelF
Copy link
Author

JelF commented Dec 17, 2015

also, there is no way to cache by block value (if it is not nil), so semantics should be as i've written in top post:

There should be either exception or no memoization in this case

@matthewrudy
Copy link
Owner

@JelF so you're literally trying to memoize Hash#fetch?

Again, what are you trying to achieve?
you might be better just using a hash

# this will cache the value in the hash when called
>> powers = Hash.new { |h, k| puts "2**#{k}"; h[k] = 2**k}

# first time the key is not there, so it hits the block
>> powers[24]
2**24
=> 16777216

# second time it just returns the value
>> powers[24]
=> 16777216

@matthewrudy
Copy link
Owner

So this code originates from 2008
rails/rails@8a9934a

It has changed a bit over the years
and I took it over in 2011.

But it's never supported blocks, and no one has asked for it too until now.

So I'm just trying to understand what you're trying to do,
so I can understand the problem.

@JelF
Copy link
Author

JelF commented Dec 17, 2015

@matthewrudy i am tried to achieve a stub i can rewite to something like

def fetch(*args, &block)
  IceNine.deep_freeze(global_config.fetch(*args, &block).with_indifferent_access)
end
memoize :fetch

And also this is a live hash, so i can not deep_freeze it somewhere except fetch method. Also, this was a wrong decision because good memoization can not be done here

If you simply raise something like Memoist::BlocksNotSupported when memozied method received block, i can understand what is wrong in a moment, but if i memoized method by mistake, and no one used it with block for a while, it will be hard to debug.

@matthewrudy
Copy link
Owner

@JelF cool.

So with the &block I can do it pretty easy.

>> def with_block_arg(&block); end
>> method(:with_block_arg).parameters
=> [[:block, :block]]

I can check for that

But with yield I don't know a way.

>> def with_yield(); yield; end
>> method(:with_yield).parameters
=> []
>> method(:with_yield).methods.sort - Object.methods
=> [:[], :arity, :call, :curry, :original_name, :owner, :parameters, :receiver, :source_location, :super_method, :to_proc, :unbind]

But yeah, let's add an error if the parameters include an &block

@JelF
Copy link
Author

JelF commented Dec 17, 2015

@matthewrudy it will break back-comp and cause problems with parameters delegation. e.g.

def bar(x)
  x + 1
end

def foo(*args, &block)
  bar(*args, &block).to_s
end
memoize :foo

JelF added a commit to JelF/memoist that referenced this issue Dec 17, 2015
JelF added a commit to JelF/memoist that referenced this issue Dec 18, 2015
@JelF JelF linked a pull request Dec 18, 2015 that will close this issue
matthewrudy pushed a commit that referenced this issue Jan 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants