Improve the Memoizable module

[matthewmcgarvey]

Fixes #1119

Purpose

This adds support for memoization of nil responses as well as using the macro with methods with arguments. I've not written any real macros before and this seems to be quite a monster so I have no idea if this is any good or not or if there are any missing edge cases.

It made sense to me to store the cached value in a tuple with all the arguments that are passed. If the method does not take any arguments but returns a string the tuple will just be Tuple(String) but if the method takes in a number it would be Tuple(String, Int32). Then, those tuple values that aren't the return value are checked against all passed in args. I didn't expect iterating over the args with a for-loop to automatically handle named arguments but my test seems to show that it does.

Example

class Users::Show < BrowserAction
  get "/users/:user_id" do
    stuff = get_user_and_make_api_call(user_id)
    # other stuff...
  end

  memoized get_user_and_make_api_call(user_id : String) : User
    get_user(user_id) && make_api_call
  end
end

becomes...

class Users::Show < BrowserAction
  get "/users/:user_id" do
    stuff = get_user_and_make_api_call(user_id)
    # other stuff...
  end

  @__memoized_get_user_and_make_api_call : Tuple(User, String)?

  def get_user_and_make_api_call__uncached(user_id : String) : User
    get_user(user_id) && make_api_call
  end

  def get_user_and_make_api_call__tuple_cached(user_id : String) : Tuple(User, String)
    @__memoized_get_user_and_make_api_call = nil if user_id != @__memoized_get_user_and_make_api_call.try &.at(1)
    @__memoized_get_user_and_make_api_call ||= -> do
      result = get_user_and_make_api_call__uncached(user_id)
      {result, user_id}
    end.call.not_nil!
  end

  def get_user_and_make_api_call(user_id : String) : User
    get_user_and_make_api_call__tuple_cached(user_id).first
  end
end

Checklist

• - An issue already exists detailing the issue/or feature request that this PR fixes
• - All specs are formatted with crystal tool format spec src
• - Inline documentation has been added and/or updated
• - Lucky builds on docker with ./script/setup
• - All builds and specs pass on docker with ./script/test

[jwoertink]

Dude! 🤩 this is amazing! I had been thinking about how to best tackle this, and I love the solution you came up with. I just left some small comments, but this is good to go!

[paulcsmith]

AMAZING work. Macros are crazy so I appreciate you showing the generated output for ease of reference.

Just a couple small tweaks I suggested, but overall this is awesome. Can't wait to get it in

[paulcsmith]

I'm not totally sure I understand this part. I think it does hold onto the value, but it creates a new value for the new arguments?

I wonder if this might just be expected and so this sentence could be left off. What do you think?

[matthewmcgarvey]

I can't tell if we're on the same page or not. Calling the memoized method with new arguments and then with the old arguments will re-run the memoized method. Does the updated description make this more clear?

[paulcsmith]

Ohhhh, so if you do my_method(1) and then my_method(2). my_method(2) will overwrite the cached value. So if you do my_method(1) again it will re-run the method.

Is that right? If so I think I got it and the doc changes look good!

[matthewmcgarvey]

Exactly 👍

[paulcsmith]

Thank you :D 🚀 ❤️