Conversation
Fixes #1042. # Conflicts: # docs/versionhistory.rst
* Handle typing of arguments to cached instance methods * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fix pre-commit issues --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
The arguments cannot be correctly inferred by any static type checker.
|
@tapetersen I'd welcome a review if you'd like to do one. |
|
@agronholm Holidays so was kind of busy but would gladly do. I keep an eye on here regularly and will add some questions at least for other MR's when I get the chance. I don't mind but am somewhat surprised you went for the route of ditching preserving type-support for arguments using ParamSpecs. It does make some sense to do the same as the existing hints for the sync version and it would indeed not be possible to annotate the classmehtod/staticmethod cases correctly for current checkers but I would probably have taken the tradeoff of those being rare special cases, with reasonably simple workarounds of simply using a module level function instead, in return for getting correct typing for useage of the function/method in the common case. |
I didn't see any signals from other potential reviewers so I merged once I got the first accepting review.
How else was I going to prevent false type checking errors in all cases (free functions, static methods, class methods, instance methods)? I believe we tried everything we could, and I could not find any examples out there which accomplishes all that. |
Correct type-checking in the staticmethod/classmethod cases was indeed what I would've given up with, my possibly very wrong, assumption that these are uncommon niche-cases that can be very reasonably worked around with module-level functions instead. If requested enough added separate decorators could also be added. |
I spent considerable time trying everything I could to make all the cases work with strict type checking. The trouble is that type checkers can't tell the difference between class methods and static methods, as they simply disregard the |
|
I don't think there is a way to make all cases work with strict type-checking and ParamSpec so what I was suggesting was indeed to accept/ignore that failure and document the straightforward work-around of instead decorating a module-level function and either just call that directly or let the static/classmethod wrap it if really needed. Alternatively we could probably get correct typing by providing our own As this module is meant as more or less straight up drop-in replacement for the builtin functools however I very well see the argument of just keeping the same behavior and typing as the builtin one which is technically correct in all but less helpful in what I would think is the more common case. |
Changes
Fixes #1042.
Checklist
If this is a user-facing code change, like a bugfix or a new feature, please ensure that
you've fulfilled the following conditions (where applicable):
tests/) which would fail without your patchdocs/), in case of behavior changes or newfeatures
docs/versionhistory.rst).If this is a trivial change, like a typo fix or a code reformatting, then you can ignore
these instructions.
Updating the changelog
If there are no entries after the last release, use
**UNRELEASED**as the version.If, say, your patch fixes issue #123, the entry should look like this:
If there's no issue linked, just link to your pull request instead by updating the
changelog after you've created the PR.