Skip to content

Fix double memoization bug #7

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.

Sign up for GitHub

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

Jump to bottom
Open
barunio wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix double memoization bug #7

barunio wants to merge 3 commits into from

Conversation

@barunio
Copy link
Contributor

@barunio barunio commented Jan 10, 2019

From the relevant commit:

Previously, calling memoize with the same method name more than once would
result in a perpetual loop when calling that method. This commit fixes the bug.
Memoizer now keeps track of which methods have been memoized and avoids trying
to memoize an already-memoized method.

tristil
tristil approved these changes Jan 10, 2019
View reviewed changes
Copy link

@tristil tristil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🍰

@bryan-ash bryan-ash self-assigned this Jan 10, 2019
bryan-ash
bryan-ash approved these changes Jan 10, 2019
View reviewed changes
Copy link
Contributor

@bryan-ash bryan-ash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🍖 🏅

barunio and others added 3 commits February 22, 2022 16:19
@barunio @fixlr
fix bug when double-memoizing methods
8fce11a 
Previously, calling `memoize` with the same method name more than once would
result in a perpetual loop when calling that method. This commit fixes the bug.
Memoizer now keeps track of which methods have been memoized and avoids trying
to memoize an already-memoized method.
@fixlr
Track memoized methods using a hash
6ca8656 
This commit changes the implementation of the
`memoizer_memoized_methods` variable to use a hash instead of an array
so that the lookup is constant time.
@barunio @fixlr
bump to version 1.0.4
de5ccae 
Only the patch version has been bumped. Changes since the previous
version should be backwards-compatible with all tested versions of ruby.

Co-authored-by: Nathan Fixler <nathan.fixler@appfolio.com>
@fixlr fixlr force-pushed the fix_double_memoization_bug branch from 5bee81b to de5ccae Compare February 23, 2022 00:26
@megatux
Copy link

megatux commented May 20, 2022

Why is this stuck?

@tristil
Copy link

tristil commented May 23, 2022

@fixlr can you see this?

@fmichaut-diff
Copy link

fmichaut-diff commented Mar 14, 2023 •
edited
Loading

Merging this would also allow for improving the unmemoize_all method. It is currently iterating over all the methods of the class, converting the names to strings, and matching against a regex for every call.

Now that we have an array of memoized methods (@memoizer_memoized_methods.keys), we could simply iterate over it

AlexWayfer added a commit to AlexWayfer/alt_memery that referenced this pull request Mar 14, 2023
@AlexWayfer
Add specs for double memoization
e744c51 
Inspired by wegowise/memoizer#7
@AlexWayfer
Copy link

AlexWayfer commented Mar 14, 2023

JFYI, I've added similar spec to my alt_memery gem and got no fails. You're free to use and I'd glad to see feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@tristil tristil tristil approved these changes

@bryan-ash bryan-ash bryan-ash approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@bryan-ash bryan-ash

Labels

Awaiting QA

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@barunio @megatux @tristil @fmichaut-diff @AlexWayfer @bryan-ash @fixlr