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
bpo-39481: Make functools.cached_property, partial, ... #19427
Conversation
... partialmethod, _lru_cache_wrapper generic
The conflicts are really annoying. I think we should merge these one at a time and then resolve conflicts in one following PR and land that before merging the next one. That will take a lot of time, because each time we must wait for the tests... :-( |
Based on this I withdraw my approval. @rhettinger what do you think? @ethanhs If you want to make progress, remove |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_lru_cache_wrapper
issue.
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 suggest omitting the lru_cache() for now. Also, it would be nice to have a few tests. |
@rhettinger Okay, sounds good, I will remove the lru_cache change, that can be done in a follow up. As for tests, I added the modified types to the GenericAlias tests that check they are subscriptable here: https://github.com/python/cpython/pull/19427/files#diff-f3accc4cfc3724113e786a2ba3548f35R27 Edit: that link seems broken, but in test_genericalias.py, there are tests for all the relevant types. Did you have other tests in mind? |
I have made the requested changes; please review again |
Thanks for making the requested changes! @gvanrossum: please review the changes made to this pull request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Raymond should be happy, so I'll merge now.
@gvanrossum: Please replace |
... partialmethod, _lru_cache_wrapper generic.
I noticed that
_lru_cache_wrapper
's Python implementation is a function, what should be done about that? I feel like it would be weird to have code that runs without the compiled functools to break on a subscripted lru_cache. Perhaps it should be a class instead?https://bugs.python.org/issue39481