matthewrudy / memoist

ActiveSupport::Memoizable with a few enhancements
MIT License
920 stars 99 forks source link

Issue with extract_reload method and optional arguments #61

Open dskim opened 7 years ago

dskim commented 7 years ago

I've just encountered this weird issue where my method is doing something unexpected. It seems to be related to extract_reload! method of Memoize.

This method can be found on https://github.com/matthewrudy/memoist/blob/master/lib/memoist.rb#L53.

Here is the code in question

if args.length == method.arity.abs + 1 && (args.last == true || args.last == :reload)
  reload = args.pop
end

It removes the last arguments from the args if it is either true or `:reload which seemingly to support the reloading of the method when it is desired.

However, the problem occurs if you have more than one optional arguments

def blah(first_arg = 'something, second_arg = true)
  #do something
end

Memoize will happily remove that second_arg thinking it is the flag for the reload when it is not so. second_arg will always be true in this case because of that behaviour of Memoize.

I can see that this is because Method#arity returns -1 in this case as there is no required arguments. I think it's unreliable to rely on arity to determine whether you can safely assume the last argument is the optional value meant for Memoize.

Obviously, I can work around this issue, but this would cause all sorts of weird issues for unsuspecting users so it'd be good to have it fixed. I'd appreciate if anyone has any thoughts on this?

Thanks,

CapCap commented 7 years ago

This is a fairly big issue I think- monkey patched in our code to be

  def self.extract_reload!(method, args)
    if args.length == method.arity.abs + 1 && args.last == :reload
      reload = args.pop
    end
    reload
  end