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

allow memoize to take multiple method names #37

Merged

Conversation

nevinera
Copy link
Contributor

@nevinera nevinera commented Jan 15, 2022

Purpose

I'm performing the upgrade to ruby-3 on a large rails project, and we used Memoist heavily. Memoist hasn't updated to support memoizing with keyword arguments in ruby 3 yet, and it looks like it might be quite difficult to do so - there have been outstanding tickets for roughly that feature for years now.

Swapping Memery in instead has been mostly seamless, but its lack of support for memoize :foo, :bar, :baz is forcing updates to several hundred files (some of our engineers disliked memoize def foo, because they felt it was too "java-ey").

Original Internal Patch

I initially just wrote a prepend-patch for my own project to support it:

# frozen_string_literal: true

module Memery
  module PluralSupportingClassMethods
    def memoize(*method_names, **kwargs)
      method_names.each do |method_name|
        super(method_name, **kwargs)
      end
    end
  end
end

Memery::ClassMethods.send(:prepend, Memery::PluralSupportingClassMethods)

And I'm happy to continue to use that.. but I figured I'd check if you were interested in adopting the change upstream.

Specs

The code change is fairly trivial, but I didn't feel like the existing test-suite structure made it easy to exercise the change - let me know if you have a preference for test structure here I should update to match (or feel free to take the PR and just rewrite it as you prefer. I have the time, but I know communication is frequently more work than coding).

This mimics the interface Memoist allowed, without any real cost, and allows
Memery to be more of a drop-in replacement
lib/memery.rb Show resolved Hide resolved
spec/memery_spec.rb Outdated Show resolved Hide resolved
@nevinera
Copy link
Contributor Author

nevinera commented Aug 2, 2022

No interest, @tycooon?

Copy link
Owner

@tycooon tycooon left a comment

Choose a reason for hiding this comment

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

Sorry, looks like I missed this PR somehow.

spec/memery_spec.rb Outdated Show resolved Hide resolved
lib/memery.rb Show resolved Hide resolved
@nevinera nevinera requested a review from tycooon August 3, 2022 20:16
spec/memery_spec.rb Outdated Show resolved Hide resolved
@tycooon
Copy link
Owner

tycooon commented Aug 4, 2022

It also looks like you have some uncovered code in the specs.

@nevinera
Copy link
Contributor Author

nevinera commented Aug 4, 2022

It also looks like you have some uncovered code in the specs.

Hmm. I didn't originally, maybe the specs I just removed did matter :-\

Edit: Oh! uncovered spec lines, I see :-)
Sorry, I'm not used to having coverage run against those at all. Does seem useful for finding unnecessary code!

@nevinera nevinera requested a review from tycooon August 4, 2022 16:28
@tycooon
Copy link
Owner

tycooon commented Aug 4, 2022

I am ready to merge, but rubocop is not happy with empty block (and I agree with that).

@nevinera
Copy link
Contributor Author

nevinera commented Aug 4, 2022

hmm, that's interesting. I ran rubocop locally, but apparently that doesn't execute the same checks your CI is performing. That's awkward - I can't define a method with an empty body, but if I put anything in the body, it'll show up as non-executed code for your coverage system :-\

Ah, I'll define those methods via attr_reader instead :-)

this is a way to satisfy the coverage and linters at the same time - we can't
define a method with an empty block, but we aren't _calling_ those methods, so
if we put any actual lines of code inside they'll end up failing the coverage
check
@tycooon tycooon merged commit 7cb9915 into tycooon:master Aug 4, 2022
@nevinera nevinera deleted the memoize-multiple-methods-in-one-call branch August 4, 2022 16:59
@nevinera
Copy link
Contributor Author

nevinera commented Aug 4, 2022

Thanks for your attention! I'll look forward to deleting one of the many monkey-patches I've had to make updating to ruby3 :-)

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