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

Document alternative ways of reloading #50

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

olivierlacan
Copy link
Contributor

I find method_name(true) isn't very intention-revealing whereas the
supported method_name(reload: true) or method_name(:reload) are
much clearer alternatives and I keep having to check the source to
see if it is possible, so why not document it in the README?

See:

memoist/lib/memoist.rb

Lines 52 to 57 in d0101e8

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

I find method_name(true) isn't very intention-revealing whereas the 
supported method_name(reload: true) or method_name(:reload) are 
much clearer alternatives and I keep having to check the source to 
see if it is possible, so why not document it in the README?

See: https://github.com/matthewrudy/memoist/blob/d0101e8bdcb129bb4b609055db0107be2c645821/lib/memoist.rb#L52-L57
@matthewrudy
Copy link
Owner

@olivierlacan does the reload: true one work?

The problem with this method is that it's confused by the fact it can be applied to a method with any arity.

@olivierlacan
Copy link
Contributor Author

@matthewrudy It does.

@sebjacobs sebjacobs force-pushed the master branch 2 times, most recently from 9d66c95 to 34f90dd Compare November 8, 2019 21:05
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