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

curry memoize by default. #146

Merged
merged 3 commits into from
Mar 19, 2014
Merged

curry memoize by default. #146

merged 3 commits into from
Mar 19, 2014

Conversation

eriknw
Copy link
Member

@eriknw eriknw commented Mar 18, 2014

This allows a cache to be provided when using memoize as a decorator.

This allows a cache to be provided when using `memoize` as a decorator.

@curry(memoize, cache={0: 0, 1: 1})
# Provide a cache with initial values to `memoize`. This works as a decorator
# because `memoize` is curried (see `toolz.curry`) by default.
Copy link
Member

Choose a reason for hiding this comment

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

I recommend skipping this explanation. I don't think many of our readers will know that this is strange and the reference to curry probably makes some people tune out.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. Should I remove the comment in the docstring too?

Copy link
Member

Choose a reason for hiding this comment

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

I say keep that one.

@eriknw
Copy link
Member Author

eriknw commented Mar 18, 2014

An argument against this PR is that toolz.curried.memoize already exists. However, I still support currying memoize, because using it as a decorator--and being able to provide a cache or other keyword arguments--is typical usage.

@mrocklin
Copy link
Member

+1

@eriknw
Copy link
Member Author

eriknw commented Mar 19, 2014

Cool. Time to merge?

@mrocklin
Copy link
Member

Merging.

I also want to ping #147 here for reference.

mrocklin added a commit that referenced this pull request Mar 19, 2014
@mrocklin mrocklin merged commit 2c1e262 into pytoolz:master Mar 19, 2014
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