Skip to content
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

Adds decorated @property support, refs #1362 #11719

Closed
wants to merge 7 commits into from
Closed

Adds decorated @property support, refs #1362 #11719

wants to merge 7 commits into from

Conversation

sobolevn
Copy link
Member

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

Closes #1362

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@sobolevn
Copy link
Member Author

sobolevn commented Dec 12, 2021

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
Copy link
Member Author

sobolevn commented Dec 12, 2021

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

@github-actions

This comment has been minimized.

@sobolevn sobolevn changed the title Adds first test to @property, refs #1362 Adds decorated @property support, refs #1362 Dec 12, 2021
@antazoey
Copy link

antazoey commented Dec 13, 2021

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
Copy link
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.

🚀 👍

@github-actions

This comment has been minimized.

@github-actions
Copy link
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
Copy link
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Decorated property not supported
2 participants