memoist icon indicating copy to clipboard operation
memoist copied to clipboard

Raises exception instead of emitting warning.

Open paneq opened this issue 6 years ago • 15 comments

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.

paneq avatar Nov 14 '18 13:11 paneq

@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.

matthewrudy avatar Nov 15 '18 12:11 matthewrudy

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.

paneq avatar Nov 15 '18 13:11 paneq

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.

barthez avatar Nov 16 '18 08:11 barthez

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

paneq avatar Nov 16 '18 08:11 paneq

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.

pboling avatar Nov 17 '18 14:11 pboling

@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 avatar Jan 22 '19 10:01 pirj

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

pirj avatar Feb 01 '19 08:02 pirj

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

pboling avatar Feb 07 '19 22:02 pboling

Ping.

pirj avatar Mar 20 '19 08:03 pirj

@matthewrudy what do you think of this change?

pirj avatar May 10 '19 14:05 pirj

Just a friendly ping one year later :)

paneq avatar Jun 05 '20 14:06 paneq

@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.

pboling avatar Jun 07 '22 19:06 pboling

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

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

pirj avatar Jun 07 '22 20:06 pirj

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 avatar Jan 27 '23 03:01 pboling

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.

pboling avatar Jun 04 '24 07:06 pboling