Use functools.cached_property on 3.8+ and for TYPE_CHECKING
#1417
Conversation
DanielNoord
added
the
Maintenance
astroid/decorators.py
Outdated
|
|
||
| After first usage, the <property_name> becomes part of the object's | ||
| __dict__. Doing: | ||
| cachedproperty = cached_property |
There was a problem hiding this comment.
Not exactly what I had in mind. That assignment will make it more not less difficult to understand that cachedproperty does the same thing functools.cached_property does.
I would suggest the replacement in the modules where it's used instead but the other way round. That should also make deprecating cachedproperty much easier.
if sys.version_info >= (3, 8) or TYPE_CHECKING:
from functools import cached_property
else:
from astroid.decorators import cachedproperty as cached_propertyThere was a problem hiding this comment.
Fine with me! Just wanted to show what I was thinking of.
I'll update the PR.
astroid/decorators.py
Outdated
| "The cachedproperty class is functionally the same as functools.cached_property " | ||
| "and will be removed after support for Python < 3.8 has been dropped. " | ||
| "Consider importing the functools variant on >= 3.8.", |
There was a problem hiding this comment.
I would actually suggest to remove it with astroid 3.0 (at least for Python 3.8+). We can define it conditionally after that point.
if sys.version_info < (3, 8):
class cachedproperty:
...My suggestion
cachedproperty has been deprecated and will be removed in astroid 3.0 for Python 3.8+.
Use functools.cached_property instead.
--
As a side note, personally I do consider cachedproperty to be internal. Thus the deprecation note is more for us than anyone else. It might even make sense to add a TODO along the lines: Remove completely when support for 3.7 is dropped.
There was a problem hiding this comment.
I didn't know if 3.0 would be 3.8+ so I didn't mention a specific version. I'll update.
astroid/nodes/node_classes.py
Outdated
| @@ -80,6 +80,11 @@ | |||
| from astroid import nodes | |||
| from astroid.nodes import LocalsDictNodeNG | |||
|
|
|||
| if sys.version_info >= (3, 8) or TYPE_CHECKING: | |||
| from functools import cached_property # pylint: disable=ungrouped-imports | |||
There was a problem hiding this comment.
| from functools import cached_property # pylint: disable=ungrouped-imports | |
| # pylint: disable-next=ungrouped-imports | |
| from functools import cached_property |
This looks like something we should fix in pylint -> ungrouped-imports 🤔
There was a problem hiding this comment.
Yeah something weird is going on here..
astroid/nodes/node_ng.py
Outdated
| @@ -38,6 +38,11 @@ | |||
| else: | |||
| from typing_extensions import Literal | |||
|
|
|||
| if sys.version_info >= (3, 8) or TYPE_CHECKING: | |||
| from functools import cached_property # pylint: disable=ungrouped-imports | |||
There was a problem hiding this comment.
| from functools import cached_property # pylint: disable=ungrouped-imports | |
| # pylint: disable-next=ungrouped-imports | |
| from functools import cached_property |
| if sys.version_info >= (3, 8) or TYPE_CHECKING: | ||
| from functools import cached_property | ||
| else: | ||
| from astroid.decorators import cachedproperty as cached_property |
There was a problem hiding this comment.
Strange that pylint: disable-next=ungrouped-imports is needed in some cases but not all 🤔
tests/unittest_decorators.py
Outdated
| @@ -97,3 +99,18 @@ def test_deprecated_default_argument_values_ok(recwarn: WarningsRecorder) -> Non | |||
| instance = SomeClass(name="some_name") | |||
| instance.func(name="", var=42) | |||
| assert len(recwarn) == 0 | |||
|
|
|||
|
|
|||
| @pytest.mark.skipif(sys.version_info < (3, 8), reason="Requires Python 3.8 or higher") | |||
There was a problem hiding this comment.
| @pytest.mark.skipif(sys.version_info < (3, 8), reason="Requires Python 3.8 or higher") | |
| @pytest.mark.skipif(not PY38_PLUS, reason="Requires Python 3.8 or higher") |
For tests it isn't necessary to use sys.version_info (for typing).
Steps
Description
This closes #1410. I guess the deprecation introduced in #1243 can wait a bit.
@cdce8p This is what I meant with redefining
cachedproperty. I'm not sure if this is something you would want to do, but I tested and this does helpmypyrecognise the types of these properties correctly.Type of Changes
Related Issue