aio-libs / propcache

Fast property caching
Apache License 2.0
7 stars 1 forks source link

Change type from Any to object for under_cached_property.__get__ #50

Closed bdraco closed 2 weeks ago

bdraco commented 2 weeks ago

If the implementation is type safe with anything, then it should be updated to object. I've had discussions in typeshed before, and the consensus is that typeshed should use object in such circumstances (otherwise, it triggers Any warnings in mypy if all the any warnings are enabled, anytime a user uses that function).

_Originally posted by @Dreamsorcerer in https://github.com/aio-libs/propcache/pull/38#discussion_r1790880159_

bdraco commented 2 weeks ago

Seems ok downstream in yarl

bdraco commented 2 weeks ago

Seems ok in aiohttp

codecov[bot] commented 2 weeks ago

Codecov Report

Attention: Patch coverage is 33.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 88.24%. Comparing base (cc02c1f) to head (883fdb7).

Files with missing lines Patch % Lines
src/propcache/_helpers_py.py 33.33% 0 Missing and 2 partials :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #50 +/- ## ======================================= Coverage 88.24% 88.24% ======================================= Files 17 17 Lines 689 689 Branches 98 98 ======================================= Hits 608 608 Misses 63 63 Partials 18 18 ``` | [Flag](https://app.codecov.io/gh/aio-libs/propcache/pull/50/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aio-libs) | Coverage Δ | | |---|---|---| | [CI-GHA](https://app.codecov.io/gh/aio-libs/propcache/pull/50/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aio-libs) | `88.24% <33.33%> (ø)` | | | [MyPy](https://app.codecov.io/gh/aio-libs/propcache/pull/50/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aio-libs) | `77.84% <33.33%> (ø)` | | | [OS-Linux](https://app.codecov.io/gh/aio-libs/propcache/pull/50/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aio-libs) | `98.38% <ø> (ø)` | | | [OS-Windows](https://app.codecov.io/gh/aio-libs/propcache/pull/50/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aio-libs) | `93.65% <ø> (ø)` | | | [OS-macOS](https://app.codecov.io/gh/aio-libs/propcache/pull/50/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aio-libs) | `94.17% <ø> (ø)` | | | [Py-3.10.11](https://app.codecov.io/gh/aio-libs/propcache/pull/50/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aio-libs) | `92.88% <ø> (ø)` | | | [Py-3.10.15](https://app.codecov.io/gh/aio-libs/propcache/pull/50/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aio-libs) | `95.46% <ø> (ø)` | | | [Py-3.11.10](https://app.codecov.io/gh/aio-libs/propcache/pull/50/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aio-libs) | `95.46% <ø> (ø)` | | | [Py-3.11.9](https://app.codecov.io/gh/aio-libs/propcache/pull/50/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aio-libs) | `92.88% <ø> (ø)` | | | [Py-3.12.6](https://app.codecov.io/gh/aio-libs/propcache/pull/50/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aio-libs) | `95.46% <ø> (ø)` | | | [Py-3.12.7](https://app.codecov.io/gh/aio-libs/propcache/pull/50/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aio-libs) | `?` | | | [Py-3.13.0-rc.3](https://app.codecov.io/gh/aio-libs/propcache/pull/50/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aio-libs) | `95.46% <ø> (ø)` | | | [Py-3.8.10](https://app.codecov.io/gh/aio-libs/propcache/pull/50/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aio-libs) | `93.18% <ø> (ø)` | | | [Py-3.8.18](https://app.codecov.io/gh/aio-libs/propcache/pull/50/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aio-libs) | `95.77% <ø> (ø)` | | | [Py-3.9.13](https://app.codecov.io/gh/aio-libs/propcache/pull/50/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aio-libs) | `92.85% <ø> (ø)` | | | [Py-3.9.20](https://app.codecov.io/gh/aio-libs/propcache/pull/50/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aio-libs) | `95.45% <ø> (ø)` | | | [Py-pypy7.3.11](https://app.codecov.io/gh/aio-libs/propcache/pull/50/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aio-libs) | `95.50% <ø> (ø)` | | | [Py-pypy7.3.16](https://app.codecov.io/gh/aio-libs/propcache/pull/50/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aio-libs) | `95.13% <ø> (ø)` | | | [Py-pypy7.3.17](https://app.codecov.io/gh/aio-libs/propcache/pull/50/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aio-libs) | `95.14% <ø> (ø)` | | | [VM-macos-latest](https://app.codecov.io/gh/aio-libs/propcache/pull/50/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aio-libs) | `94.17% <ø> (ø)` | | | [VM-ubuntu-latest](https://app.codecov.io/gh/aio-libs/propcache/pull/50/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aio-libs) | `98.38% <ø> (ø)` | | | [VM-windows-latest](https://app.codecov.io/gh/aio-libs/propcache/pull/50/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aio-libs) | `93.65% <ø> (ø)` | | | [pytest](https://app.codecov.io/gh/aio-libs/propcache/pull/50/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aio-libs) | `98.38% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aio-libs#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Dreamsorcerer commented 2 weeks ago

For calling code, this change could only remove errors, not introduce them (i.e. if using --warn-any-expr).

bdraco commented 2 weeks ago

Before MyPy flags All Flags

After MyPy flags All Flags

bdraco commented 2 weeks ago

Super confusing that Coverage not affected when comparing cc02c1f...883fdb7 in the CI checks and ..

Attention: Patch coverage is 33.33333% with 2 lines in your changes missing coverage. Please review. https://github.com/aio-libs/propcache/pull/50#issuecomment-2397931538

bdraco commented 2 weeks ago

https://mypy.readthedocs.io/en/latest/dynamic_typing.html#any-vs-object

If you’re not sure whether you need to use object or Any, use object – only switch to using Any if you get a type checker complaint.

I'd say we didn't get any complaints so that seems to be the prescribed way to go.

bdraco commented 2 weeks ago

Too tired and jet lagged to revalidate so I'll leave this for someone else to ✅ and merge or I'll take a look again when I've had some sleep.

bdraco commented 2 weeks ago

I think this is good to merge as-is, but I wasn't 100% on all the discussions above so I'll leave it until they are resolved above.

webknjaz commented 2 weeks ago

I think it's okay to merge. This is probably best we can do right now.