Raises exception instead of emitting warning.

paneq commented 5 years ago

The message tries to guide the developer on how to workaround the issue.

It's better to fail rather than skip memoization which can lead to bugs.

matthewrudy commented 5 years ago

@paneq thanks for the work.

Any idea why it fails on the old rubies?

I like the idea of giving more information, but why change from a warning to an exception?

I'm not keen to make any breaking changes.

paneq commented 5 years ago

Any idea why it fails on the old rubies?

nope

why change from a warning to an exception?

noticing a warning might be hard (foreman or docker with tons of other output)
noticing a warning does not automatically lead to fixing it by a developer who does not understand where it is coming from and what to do
memoizing is not always used for performance speedup. In our cases it was used for caching the calls which return random results, which should be reused. So essentially the current design leads to hidden bugs.

I'm not keen to make any breaking changes.

Assuming developers don't add memoize in runtime and they eager load entire codebase (ie rails app in testing) they should get this exception (with instructions on how to fix it after upgrade) when running tests.

They can even apply the recommended changes in the code using older gem version, and then upgrade to a newer version which won't crash.

I am not particularly interested in supporting developers who:

don't test
their tests don't load entire codebase
dynamically create subclasses with memoization (not working in fact) in runtime

On the other hand I prefer to raise an exception, when a guaranteed bug occurs, for the rest of the developers out there so that they can notice it, the moment they introduced it and fix accordingly.

Up to you.

I spent plenty of time looking at code with memoize in front of it, thinking that it memoizes the value and wondering what is wrong and where a certain bug comes from, only to discover that the value is not memoized at all.

So this is where my recommendation comes from.

barthez commented 5 years ago

Test fails on Ruby < 2.1 due def behavior change (https://bugs.ruby-lang.org/issues/3753). Until Ruby 2.1 def was not returning defined method name as symbol, MRI was returning nil.

Tests needs to be fixed to call memoize :method_name after method definition.

paneq commented 5 years ago

@barthez https://www.ruby-lang.org/en/news/2017/04/01/support-of-ruby-2-1-has-ended/

pboling commented 5 years ago

This gem should drop older Rubies eventually because eventually supporting them will be detrimental to forward progress. But if supporting them is not yet detrimental, then there is little reason to break the support.

pirj commented 5 years ago

@matthewrudy @pboling Fixed Ruby 2.1 support, build is green now.

Do you think it's ok to introduce breaking changes even though the gem has not reached 1.0 yet?

pirj commented 5 years ago

@matthewrudy @pboling What are your thoughts on this guys?

pboling commented 5 years ago

@matthewrudy @pirj I love this change. I prefer noisy failure, early, and as often as possible when things are legit broken.

pirj commented 5 years ago

Ping.

pirj commented 5 years ago

@matthewrudy what do you think of this change?

paneq commented 4 years ago

Just a friendly ping one year later :)

pboling commented 2 years ago

@paneq @pirj Matthew, the owner of this repo, died in 2019. I recently found out. I have created a new org in his memory, called memoist. I'm adding you to it (don't have to accept if you'd rather not).

Matthew's brother, @sebjacobs, maintained the repo for a while through 2020, but it isn't clear if he is still involved in open source at this point.

pirj commented 2 years ago

Thanks, @pboling. Sad to hear that about Matthew.

Unfortunately enough, I'm out of additional capacity ATM to maintain memoist.

pboling commented 1 year ago

I copied Matthew's repos to a new @memoist org, but it is looking like they would have to be hard forks into a new gem namespace to continue.

In the meantime I am switching to https://github.com/makandra/memoized

pboling commented 3 months ago

FYI: Added this alert to the new memoist repo

[!IMPORTANT]

Recommendation

Consider using MemoWise instead, as it is maintained, fully tested, provides thread safety guarantees, and is much, much faster.

Other Alternatives

In case you need a tool with this feature set that is currently maintained, check out:

https://github.com/makandra/memoized

https://github.com/honzasterba/memoist

https://github.com/tycooon/memery

[!TIP]
Seriously though, read the important note above.

[!WARNING]
If you must continue - be aware that this is unmaintained software.

matthewrudy / memoist

Raises exception instead of emitting warning. #78

Recommendation

Other Alternatives