Skip to content

bpo-38565: add new cache_parameters method for lru_cache #16916

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.

Sign up for GitHub

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

Jump to bottom
Merged
rhettinger merged 6 commits into from
Nov 12, 2019
Merged

bpo-38565: add new cache_parameters method for lru_cache #16916

rhettinger merged 6 commits into from
Nov 12, 2019

Conversation

@Zheaoli
Copy link
Contributor

@Zheaoli Zheaoli commented Oct 24, 2019 •
edited by bedevere-bot
Loading

@Zheaoli Zheaoli requested a review from rhettinger as a code owner October 24, 2019 08:10
rhettinger
rhettinger requested changes Oct 29, 2019
View reviewed changes
Lib/functools.py Outdated
Copy link
Contributor

@rhettinger rhettinger Oct 29, 2019

Choose a reason for hiding this comment

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

NIt: Please use single quotes, the same as used by dict.__repr__()

Copy link
Contributor

@rhettinger rhettinger Oct 29, 2019

Choose a reason for hiding this comment

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

Also move the function to just after cache_clear().

Lib/test/test_functools.py Outdated
Copy link
Contributor

@rhettinger rhettinger Oct 29, 2019

Choose a reason for hiding this comment

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

Let's use the decorator notation to create the example. Also, make the example use maxsize=1000 and typed=True.

Misc/NEWS.d/next/Library/2019-10-24-08-10-30.bpo-38565.SWSUst.rst Outdated
Copy link
Contributor

@rhettinger rhettinger Oct 29, 2019

Choose a reason for hiding this comment

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

``Add new cache_parameters() method for functools.lru_cache() to better support pickling.

Modules/_functoolsmodule.c Outdated
Copy link
Contributor

@rhettinger rhettinger Oct 29, 2019

Choose a reason for hiding this comment

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

Need to test whether cache_parameters is NULL.

Modules/_functoolsmodule.c Outdated
Copy link
Contributor

@rhettinger rhettinger Oct 29, 2019

Choose a reason for hiding this comment

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

There are also two possible failure points that need to be tested here as well.

Modules/_functoolsmodule.c Outdated
Copy link
Contributor

@rhettinger rhettinger Oct 29, 2019

Choose a reason for hiding this comment

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

  1. Drop the test for zero, putting the positive case first: self->typed ? Py_True : Py_False)
  2. Test the result of PyDict_SetItemString() to see if it failed.

Modules/_functoolsmodule.c Outdated
Copy link
Contributor

@rhettinger rhettinger Oct 29, 2019

Choose a reason for hiding this comment

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

Move this down one, just after the cache_clear entry.

@bedevere-bot
Copy link

bedevere-bot commented Oct 29, 2019

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@Zheaoli Zheaoli force-pushed the bpo-38565 branch 2 times, most recently from 8354f2d to 37f4f0d Compare October 30, 2019 17:11
@Zheaoli
Copy link
Contributor Author

Zheaoli commented Oct 30, 2019

I have made the requested changes; please review again

@bedevere-bot
Copy link

bedevere-bot commented Oct 30, 2019

Thanks for making the requested changes!

@rhettinger: please review the changes made to this pull request.

@Zheaoli
Copy link
Contributor Author

Zheaoli commented Nov 9, 2019

ping @rhettinger

@rhettinger rhettinger merged commit 051ff52 into python:master Nov 12, 2019
jacobneiltaylor pushed a commit to jacobneiltaylor/cpython that referenced this pull request Dec 5, 2019
@Zheaoli
bpo-38565: add new cache_parameters method for lru_cache (pythonGH-16916
fae8f19 
)
shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020
@Zheaoli @shihai1991
bpo-38565: add new cache_parameters method for lru_cache (pythonGH-16916
36f13ab 
)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rhettinger rhettinger rhettinger approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Zheaoli @bedevere-bot @rhettinger @the-knights-who-say-ni