Skip to content

Add proper memo_wise initialization for inherited classes#392

Open
Galathius wants to merge 1 commit into
panorama-ed:mainfrom
Galathius:support-inherited-classes
Open

Add proper memo_wise initialization for inherited classes#392
Galathius wants to merge 1 commit into
panorama-ed:mainfrom
Galathius:support-inherited-classes

Conversation

@Galathius

@Galathius Galathius commented May 31, 2025

Copy link
Copy Markdown

Hopefully it solves the issue from this comment.

This change should allow such usage:

class BaseClass
  prepend MemoWise
end

class A < BaseClass
  def initialize; end # <--- important part, there is no `super`

  memo_wise def slow
    sleep 2
    puts 'slow result'
  end
end

Now it's failing with:

/Users/galathius/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/memo_wise-1.13.0/lib/memo_wise.rb:174:in `slow': undefined method `fetch' for nil:NilClass (NoMethodError)
	from memo_wise_test.rb:26:in `block in <main>'
	from /Users/galathius/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/benchmark-0.4.1/lib/benchmark.rb:305:in `measure'
	from memo_wise_test.rb:26:in `<main>'

Before merging:

  • Copy the table printed at the end of the latest benchmark results into the README.md and update this PR
  • If this change merits an update to CHANGELOG.md, add an entry following Keep a Changelog guidelines with semantic versioning

@Galathius Galathius force-pushed the support-inherited-classes branch 6 times, most recently from 82a3cd9 to 74c4b42 Compare June 1, 2025 20:47
Comment thread README.md

Results using Ruby 3.4.3:

|Method arguments|`alt_memery` (2.1.0)|`dry-core`\* (1.1.0)|`memery` (1.7.0)|`memoist3` (1.0.0)|`short_circu_it` (0.29.3)|

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Method arguments memo_wise-github-main (1.13.0)
() (none) 1.00x
(a) 1.00x
(a, b) 0.98x
(a:) 0.99x
(a:, b:) 1.01x
(a, b:) 1.00x
(a, *args) 1.05x
(a:, **kwargs) 0.97x
(a, *args, b:, **kwargs) 1.00x

@codecov

codecov Bot commented Jun 2, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (6d82e42) to head (74c4b42).

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #392   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            3         3           
  Lines          197       200    +3     
  Branches        89        89           
=========================================
+ Hits           197       200    +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@JacobEvelyn

JacobEvelyn commented Jun 2, 2025

Copy link
Copy Markdown
Member

Thanks for the PR, @Galathius ! I'm not sure this is right, though—your tests pass even when I remove the def inherited you added, and it appears that CreateMemoWiseStateOnInherited exists to provide this functionality but clearly isn't working correctly. I'll dig and see if I can find a more robust approach.

Edit: I actually don't understand the issue you're encountering; see #382 (comment)

@Galathius Galathius force-pushed the support-inherited-classes branch from 74c4b42 to c20455c Compare June 2, 2025 15:44
@Galathius

Copy link
Copy Markdown
Author

Hi @JacobEvelyn! Sorry, seems I accidentally removed def initialize; end from the child class in tests.

Just updated it, now if you comment my code in memo_wise.rb you'll see it's failing.

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.

2 participants