-
Notifications
You must be signed in to change notification settings - Fork 99
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
Comments
@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 But I'm not sure what a real use of this is, and therefore don't know what the correct behaviour should be. |
@matthewrudy i tried to memoize #fetch, delegated to some hash, and got unexpected KeyError in specs
|
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 |
@JelF you're correct, sorry. |
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? |
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:
|
@JelF so you're literally trying to memoize Again, what are you trying to achieve?
|
So this code originates from 2008 It has changed a bit over the years 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, |
@matthewrudy i am tried to achieve a stub i can rewite to something like
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. |
@JelF cool. So with the
I can check for that But with
But yeah, let's add an error if the |
@matthewrudy it will break back-comp and cause problems with parameters delegation. e.g.
|
There should be either exception or no memoization in this case
The text was updated successfully, but these errors were encountered: