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

Raises exception instead of emitting warning. #78

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

paneq
Copy link

@paneq paneq commented Nov 14, 2018

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.

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
Copy link
Owner

@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
Copy link
Author

paneq commented Nov 15, 2018

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
Copy link

barthez commented Nov 16, 2018

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
Copy link
Author

paneq commented Nov 16, 2018

@pboling
Copy link
Contributor

pboling commented Nov 17, 2018

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
Copy link

pirj commented Jan 22, 2019

@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
Copy link

pirj commented Feb 1, 2019

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

@pboling
Copy link
Contributor

pboling commented Feb 7, 2019

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

@pirj
Copy link

pirj commented Mar 20, 2019

Ping.

@pirj
Copy link

pirj commented May 10, 2019

@matthewrudy what do you think of this change?

@sebjacobs sebjacobs force-pushed the master branch 2 times, most recently from 9d66c95 to 34f90dd Compare November 8, 2019 21:05
@paneq
Copy link
Author

paneq commented Jun 5, 2020

Just a friendly ping one year later :)

@pboling
Copy link
Contributor

pboling commented Jun 7, 2022

@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
Copy link

pirj commented Jun 7, 2022

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

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

@pboling
Copy link
Contributor

pboling commented Jan 27, 2023

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
Copy link
Contributor

pboling commented Jun 4, 2024

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:

Tip

Seriously though, read the important note above.

Warning

If you must continue - be aware that this is unmaintained software.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants