FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Adds decorated `@property` support, refs #1362 by sobolevn · Pull Request #11719 · python/mypy · GitHub

  1. python
  2. mypy

Repository navigation

Adds decorated @property support, refs #1362 - #11719

Closed
sobolevn wants to merge 7 commits into
python:masterfrom
sobolevn:issue-1362
ConversationCommits (7)ChecksFiles changed
Closed

Adds decorated @property support, refs #1362#11719
sobolevn wants to merge 7 commits into
python:masterfrom
sobolevn:issue-1362

Conversation

sobolevn commented Dec 12, 2021

Copy link
Copy Markdown
Member

This is not fully ready right now, but looks like the first test passes 🎉

Closes #1362

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

sobolevn commented Dec 12, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

Things TODO:

  • Ensure that @property is the last (the top-most) decorator in chain. This is a slight limitation we have, but this I cannot think of use-cases when this is used differently
  • Add support for decorated @property.setter
  • @final @property support
  • Add better error message for @overload and @property used together. I don't plan to support this feature in this PR, but I guess overloded properties can be a thing later, since we can have different self types

sobolevn commented Dec 12, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

Friendly ping to @unparalleled-js. You offered your help, so maybe you would have some time to review my code 🙂

This comment has been minimized.

Sign in to view
sobolevn changed the title Adds first test to @property, refs #1362 Adds decorated @property support, refs #1362 Dec 12, 2021
antazoey reviewed Dec 12, 2021
View reviewed changes
Comment thread mypy/checker.py

antazoey commented Dec 13, 2021 •
edited
Loading

Copy link
Copy Markdown

I tested it out for both @property and @cached_property and ran mypy and some other projects, as a manual regression.

I am not sure about the other test failures yet but I can keep diving in, this is my first time peaking around mypy but I am invested.

sobolevn commented Dec 13, 2021

Copy link
Copy Markdown
Member Author

I tested it out for both @Property and @cached_property and ran mypy and some other projects, as a manual regression.

Thank you! Any interesting finds? 🙂

I am not sure about the other test failures

Test failures are about incorrect test fixture and explicit Any in it. I will fix it later today. It is not related to the feature itself (I hope).

this is my first time peaking around mypy but I am invested.

🚀 👍

This comment has been minimized.

Sign in to view

github-actions Bot commented Dec 19, 2021

Copy link
Copy Markdown
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

twine (https://github.com/pypa/twine)
+ twine/auth.py:32: error: Unused "type: ignore" comment
+ twine/auth.py:42: error: Unused "type: ignore" comment

steam.py (https://github.com/Gobot1234/steam.py)
+ steam/ext/commands/converters.py:277: error: Property must be used as the top-most decorator  [misc]

kopf (https://github.com/nolar/kopf)
+ kopf/_core/actions/invocation.py:59: error: Property must be used as the top-most decorator
+ kopf/_core/actions/invocation.py:64: error: Property must be used as the top-most decorator
+ kopf/_core/actions/invocation.py:69: error: Property must be used as the top-most decorator

core (https://github.com/home-assistant/core)
+ homeassistant/components/sensor/__init__.py:300: error: Property must be used as the top-most decorator  [misc]
+ homeassistant/components/sensor/__init__.py:341: error: Property must be used as the top-most decorator  [misc]
+ homeassistant/components/sensor/__init__.py:359: error: Property must be used as the top-most decorator  [misc]
+ homeassistant/components/light/__init__.py:882: error: Property must be used as the top-most decorator  [misc]
+ homeassistant/components/vacuum/__init__.py:294: error: Property must be used as the top-most decorator  [misc]
+ homeassistant/components/switch/__init__.py:133: error: Property must be used as the top-most decorator  [misc]
+ homeassistant/components/siren/__init__.py:156: error: Property must be used as the top-most decorator  [misc]
+ homeassistant/components/remote/__init__.py:175: error: Property must be used as the top-most decorator  [misc]
+ homeassistant/components/lock/__init__.py:160: error: Property must be used as the top-most decorator  [misc]
+ homeassistant/components/lock/__init__.py:170: error: Property must be used as the top-most decorator  [misc]
+ homeassistant/components/image_processing/__init__.py:179: error: Property must be used as the top-most decorator  [misc]
+ homeassistant/components/geo_location/__init__.py:84: error: Property must be used as the top-most decorator  [misc]
+ homeassistant/components/fan/__init__.py:462: error: Property must be used as the top-most decorator  [misc]
+ homeassistant/components/device_tracker/legacy.py:686: error: Property must be used as the top-most decorator  [misc]
+ homeassistant/components/device_tracker/config_entry.py:124: error: Property must be used as the top-most decorator  [misc]
+ homeassistant/components/device_tracker/config_entry.py:168: error: Property must be used as the top-most decorator  [misc]
+ homeassistant/components/cover/__init__.py:245: error: Property must be used as the top-most decorator  [misc]
+ homeassistant/components/calendar/__init__.py:145: error: Property must be used as the top-most decorator  [misc]
+ homeassistant/components/binary_sensor/__init__.py:202: error: Property must be used as the top-most decorator  [misc]
+ homeassistant/components/alarm_control_panel/__init__.py:212: error: Property must be used as the top-most decorator  [misc]
+ homeassistant/components/air_quality/__init__.py:134: error: Property must be used as the top-most decorator  [misc]
+ homeassistant/components/weather/__init__.py:205: error: Property must be used as the top-most decorator  [misc]
+ homeassistant/components/water_heater/__init__.py:163: error: Property must be used as the top-most decorator  [misc]
+ homeassistant/components/water_heater/__init__.py:197: error: Property must be used as the top-most decorator  [misc]
+ homeassistant/components/norway_air/air_quality.py:99: error: Unused "type: ignore" comment
+ homeassistant/components/norway_air/air_quality.py:105: error: Unused "type: ignore" comment
+ homeassistant/components/norway_air/air_quality.py:111: error: Unused "type: ignore" comment
+ homeassistant/components/norway_air/air_quality.py:117: error: Unused "type: ignore" comment
+ homeassistant/components/norway_air/air_quality.py:123: error: Unused "type: ignore" comment
+ homeassistant/components/media_player/__init__.py:920: error: Property must be used as the top-most decorator  [misc]
+ homeassistant/components/climate/__init__.py:254: error: Property must be used as the top-most decorator  [misc]
+ homeassistant/components/camera/__init__.py:574: error: Property must be used as the top-most decorator  [misc]

scikit-learn (https://github.com/scikit-learn/scikit-learn)
+ sklearn/utils/tests/test_deprecation.py:23: error: Property must be used as the top-most decorator
+ sklearn/preprocessing/_polynomial.py:495: error: Property must be used as the top-most decorator
+ sklearn/preprocessing/_data.py:2270: error: Property must be used as the top-most decorator
+ sklearn/svm/_base.py:129: error: Property must be used as the top-most decorator
+ sklearn/decomposition/_kernel_pca.py:283: error: Property must be used as the top-most decorator
+ sklearn/decomposition/_kernel_pca.py:293: error: Property must be used as the top-most decorator
+ sklearn/decomposition/_kernel_pca.py:302: error: Property must be used as the top-most decorator
+ sklearn/cluster/_kmeans.py:1740: error: Property must be used as the top-most decorator
+ sklearn/cluster/_kmeans.py:1748: error: Property must be used as the top-most decorator
+ sklearn/cluster/_kmeans.py:1756: error: Property must be used as the top-most decorator
+ sklearn/neighbors/_base.py:588: error: Property must be used as the top-most decorator
+ sklearn/manifold/_mds.py:460: error: Property must be used as the top-most decorator
+ sklearn/cluster/_affinity_propagation.py:425: error: Property must be used as the top-most decorator
+ sklearn/neighbors/_regression.py:188: error: Property must be used as the top-most decorator
+ sklearn/model_selection/_search.py:420: error: Property must be used as the top-most decorator
+ sklearn/covariance/_graph_lasso.py:1017: error: Property must be used as the top-most decorator
+ sklearn/covariance/_graph_lasso.py:1037: error: Property must be used as the top-most decorator
+ sklearn/manifold/_spectral_embedding.py:541: error: Property must be used as the top-most decorator
+ sklearn/cluster/_spectral.py:740: error: Property must be used as the top-most decorator
+ sklearn/cluster/_birch.py:485: error: Property must be used as the top-most decorator
+ sklearn/cluster/_birch.py:494: error: Property must be used as the top-most decorator
+ sklearn/pipeline.py:728: error: Property must be used as the top-most decorator
+ sklearn/naive_bayes.py:500: error: Property must be used as the top-most decorator
+ sklearn/naive_bayes.py:705: error: Property must be used as the top-most decorator
+ sklearn/naive_bayes.py:718: error: Property must be used as the top-most decorator
+ sklearn/naive_bayes.py:735: error: Property must be used as the top-most decorator
+ sklearn/multiclass.py:554: error: Property must be used as the top-most decorator
+ sklearn/multiclass.py:573: error: Property must be used as the top-most decorator
+ sklearn/multiclass.py:586: error: Property must be used as the top-most decorator
+ sklearn/multiclass.py:905: error: Property must be used as the top-most decorator
+ sklearn/kernel_ridge.py:166: error: Property must be used as the top-most decorator
+ sklearn/dummy.py:458: error: Property must be used as the top-most decorator
+ sklearn/dummy.py:705: error: Property must be used as the top-most decorator
+ sklearn/tree/_classes.py:1005: error: Property must be used as the top-most decorator
+ sklearn/tree/_classes.py:1325: error: Property must be used as the top-most decorator
+ sklearn/cross_decomposition/_pls.py:504: error: Property must be used as the top-most decorator
+ sklearn/cross_decomposition/_pls.py:512: error: Property must be used as the top-most decorator
+ sklearn/cross_decomposition/_pls.py:520: error: Property must be used as the top-most decorator
+ sklearn/cross_decomposition/_pls.py:528: error: Property must be used as the top-most decorator
+ sklearn/cross_decomposition/_pls.py:536: error: Property must be used as the top-most decorator
+ sklearn/cross_decomposition/_pls.py:1090: error: Property must be used as the top-most decorator
+ sklearn/cross_decomposition/_pls.py:1100: error: Property must be used as the top-most decorator
+ sklearn/cross_decomposition/_pls.py:1108: error: Property must be used as the top-most decorator
+ sklearn/cross_decomposition/_pls.py:1116: error: Property must be used as the top-most decorator
+ sklearn/cross_decomposition/_pls.py:1124: error: Property must be used as the top-most decorator
+ sklearn/cross_decomposition/_pls.py:1132: error: Property must be used as the top-most decorator
+ sklearn/feature_selection/_rfe.py:758: error: Property must be used as the top-most decorator
+ sklearn/ensemble/_forest.py:648: error: Property must be used as the top-most decorator
+ sklearn/ensemble/_bagging.py:471: error: Property must be used as the top-most decorator
+ sklearn/ensemble/_gb.py:895: error: Property must be used as the top-most decorator
+ sklearn/ensemble/_gb.py:1933: error: Property must be used as the top-most decorator

pandas (https://github.com/pandas-dev/pandas)
+ pandas/core/base.py:198: error: Property must be used as the top-most decorator  [misc]
+ pandas/core/base.py:214: error: Property must be used as the top-most decorator  [misc]
+ pandas/core/base.py:219: error: Property must be used as the top-most decorator  [misc]
+ pandas/core/indexes/base.py:841: error: Property must be used as the top-most decorator  [misc]
+ pandas/core/indexes/base.py:2096: error: Property must be used as the top-most decorator  [misc]
+ pandas/core/indexes/base.py:2138: error: Property must be used as the top-most decorator  [misc]
+ pandas/core/indexes/base.py:2156: error: Property must be used as the top-most decorator  [misc]
+ pandas/core/indexes/base.py:2181: error: Property must be used as the top-most decorator  [misc]
+ pandas/core/indexes/base.py:2566: error: Property must be used as the top-most decorator  [misc]
+ pandas/core/indexes/base.py:4720: error: Unused "type: ignore" comment
+ pandas/core/indexes/base.py:6831: error: Property must be used as the top-most decorator  [misc]
+ pandas/core/arrays/interval.py:1394: error: Unused "type: ignore" comment
+ pandas/core/indexes/numeric.py:272: error: Unused "type: ignore" comment
+ pandas/core/indexes/period.py:182: error: Unused "type: ignore" comment
+ pandas/core/indexes/period.py:189: error: Unused "type: ignore" comment
+ pandas/core/indexes/period.py:196: error: Unused "type: ignore" comment
+ pandas/core/internals/base.py:159: error: Property must be used as the top-most decorator  [misc]
+ pandas/io/parsers/base_parser.py:314: error: Property must be used as the top-most decorator  [misc]
+ pandas/core/series.py:721: error: Unused "type: ignore" comment
+ pandas/core/series.py:722: error: Property must be used as the top-most decorator  [misc]
+ pandas/core/generic.py:350: error: Property must be used as the top-most decorator  [misc]
+ pandas/core/generic.py:472: error: Property must be used as the top-most decorator  [misc]
+ pandas/core/generic.py:4095: error: Property must be used as the top-most decorator  [misc]
+ pandas/core/generic.py:5640: error: Property must be used as the top-most decorator  [misc]
+ pandas/core/internals/blocks.py:165: error: Property must be used as the top-most decorator  [misc]
+ pandas/core/internals/blocks.py:177: error: Property must be used as the top-most decorator  [misc]
+ pandas/core/internals/blocks.py:188: error: Property must be used as the top-most decorator  [misc]
+ pandas/core/internals/blocks.py:200: error: Property must be used as the top-most decorator  [misc]
+ pandas/core/internals/blocks.py:237: error: Property must be used as the top-most decorator  [misc]
+ pandas/core/groupby/grouper.py:301: error: Property must be used as the top-most decorator  [misc]
+ pandas/core/groupby/grouper.py:419: error: Property must be used as the top-most decorator  [misc]
+ pandas/core/groupby/groupby.py:597: error: Property must be used as the top-most decorator  [misc]
+ pandas/core/groupby/groupby.py:605: error: Property must be used as the top-most decorator  [misc]
+ pandas/core/groupby/groupby.py:610: error: Property must be used as the top-most decorator  [misc]
+ pandas/core/groupby/groupby.py:676: error: Property must be used as the top-most decorator  [misc]
+ pandas/core/groupby/groupby.py:1735: error: Property must be used as the top-most decorator  [misc]
+ pandas/core/groupby/ops.py:735: error: Property must be used as the top-most decorator  [misc]
+ pandas/core/groupby/ops.py:792: error: Property must be used as the top-most decorator  [misc]
+ pandas/core/groupby/ops.py:827: error: Property must be used as the top-most decorator  [misc]
+ pandas/core/groupby/ops.py:842: error: Property must be used as the top-most decorator  [misc]
+ pandas/core/groupby/ops.py:865: error: Property must be used as the top-most decorator  [misc]
+ pandas/core/groupby/ops.py:876: error: Property must be used as the top-most decorator  [misc]

sobolevn commented Dec 19, 2021

Copy link
Copy Markdown
Member Author

Ok, I have to admit that my .property_decorator hack didn't work. We have too many possible decorators that can go on top: @final, @no_type_check, @custom_cache, @abtract, etc.

Fixing one problem and introducing another one is not ideal.

New plan: @property is just a regular descriptor with __get__ and __set__ methods. I guess I should concentrate on making it work as it should.

sobolevn mentioned this pull request Dec 19, 2021
Closed
2 of 4 tasks
sobolevn marked this pull request as draft January 13, 2022 06:56
sobolevn closed this Feb 22, 2022
graingert mentioned this pull request Jun 22, 2022
Closed
graingert mentioned this pull request Aug 14, 2022
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
antazoey antazoey left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Decorated property not supported

2 participants

Back | FazBrowse Home | New Git URL