bpo-34995: Maintain func.__isabstractmethod__ in functools.cached_property#9904
bpo-34995: Maintain func.__isabstractmethod__ in functools.cached_property#9904mwilbz wants to merge 6 commits intopython:masterfrom
Conversation
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA). Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. You can check yourself to see if the CLA has been received. Thanks again for your contribution, we look forward to reviewing it! |
Lib/functools.py
Outdated
| self.func = func | ||
| self.attrname = None | ||
| self.__doc__ = func.__doc__ | ||
| self.__isabstractmethod__ = getattr(func, '__isabstractmethod__', False) |
There was a problem hiding this comment.
Would it be possible to only set the attribute (on self) if the attribute exists on func?
There was a problem hiding this comment.
Sure! Updated the implementation
| def calculate(self): | ||
| pass | ||
|
|
||
| with self.assertRaises(TypeError): |
There was a problem hiding this comment.
Would it be possible to (also) directly check the isabstractmethod attribute?
There was a problem hiding this comment.
Good suggestion, added an assertion for that
Lib/functools.py
Outdated
| self.func = func | ||
| self.attrname = None | ||
| self.__doc__ = func.__doc__ | ||
| func_isabstractmethod = getattr(func, '__isabstractmethod__') |
There was a problem hiding this comment.
Used like that, getattr() raises an exception if the attribute doesn't exist.
You can use:
try: self.__isabstractmethod__ = func.__isabstractmethod__
except AttributeError: pass
There was a problem hiding this comment.
Ah, thank you. I opened the implementation in my IDE and it said default=None so I was mislead. Pushing the version you're suggesting
Lib/test/test_functools.py
Outdated
| def calculate(self): | ||
| pass | ||
|
|
||
| self.assertTrue(getattr(AbstractExpensiveCalculator.calculate, '__isabstractmethod__', False)) |
There was a problem hiding this comment.
self.assertTrue(AbstractExpensiveCalculator.calculate.isabstractmethod) is enough.
| self.assertTrue(getattr(AbstractExpensiveCalculator.calculate, '__isabstractmethod__', False)) | ||
| with self.assertRaises(TypeError): | ||
| AbstractExpensiveCalculator() | ||
|
|
There was a problem hiding this comment.
Can you add a test on a method which doesn't have the __isabstractmethod__ attribute? I expect such test:
self.assertFalse(hasattr(AbstractExpensiveCalculator.calculate, '__isabstractmethod__'))
| @@ -0,0 +1 @@ | |||
| Maintain wrapped method's __isabstractmethod__ in functools.cached_property | |||
There was a problem hiding this comment.
I suggest:
:func:`functools.cached_property` now also copies the wrapped method's ``__isabstractmethod__`` attribute.
|
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 Thank you for your comments @vstinner ! Good suggestions around better test coverage + documentation. :) |
|
Thanks for making the requested changes! @vstinner: please review the changes made to this pull request. |
vstinner
left a comment
vstinner
left a comment
There was a problem hiding this comment.
LGTM.
But it seems like @serhiy-storchaka and @methane dislike the overall idea: https://bugs.python.org/issue34995
serhiy-storchaka
left a comment
serhiy-storchaka
left a comment
There was a problem hiding this comment.
I still think that it is better to use @property instead of @cached_property in abstract classes, but I have no strong objections. What would say ABC experts?
Lib/test/test_functools.py
Outdated
| def calculate(self): | ||
| pass | ||
|
|
||
| self.assertTrue(AbstractExpensiveCalculator.calculate.__isabstractmethod__) |
There was a problem hiding this comment.
Too long line. Maybe make the class or property name a little shorter?
|
Possibly outside the scope of this change, but I noticed that Is it worth considering more generally modifying the standard fields to update via |
|
Regardless of my previous comment, what are the next steps to move forward with this PR? Anything I can do? Thanks again for the reviews up until now =) |
|
It sounds like there isn't interest from core contributors in merging this change, so I'll close the PR. Thanks reviewers for your time and feedback! |
5 participants
bpo-34995: Maintain func.isabstractmethod in functools.cached_property
https://bugs.python.org/issue34995