-
Notifications
You must be signed in to change notification settings - Fork 59
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
Use ParamSpec for wrapped signatures #508
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #508 +/- ##
==========================================
+ Coverage 96.88% 96.92% +0.03%
==========================================
Files 12 12
Lines 770 780 +10
Branches 91 93 +2
==========================================
+ Hits 746 756 +10
Misses 22 22
Partials 2 2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good step forward, thanks!
I'll review this soonish and see if I can get the last issues resolved. |
@Dreamsorcerer As someone who would like to see these features get added myself, I also made my own version before seeing this Pull Request that injects _Coro = Coroutine[Any, Any, _R]
_CB = Callable[_P, _Coro[_R]]
_CBOM = Union[_CB[_P, _R], _CB[Concatenate[Self, _P], _R]]
_CBP = Union[_CBOM[_P, _R], "partial[_Coro[_R]]", "partialmethod[_Coro[_R]]"] I call For some reason it didn't branch to your pull request specifically but I hope my additions to your pull request will at least excite you. I do give you credit though for coming up with this solution to begin with. |
OK, there's still some details not fully working with partial, but I think we can merge without that. It's just the issue with methods that is blocking us now. I'm not sure how to fix it, it seems to me like the code is a little too complex for static typing. The main issue seems to be this function which dynamically changes for an instance method: async-lru/async_lru/__init__.py Lines 227 to 233 in 0f20348
The only thing I can think of is to try and split that class into 2 subclasses, rather than handling the logic within that get method. Then have a class that doesn't handle instance methods and another that does. The latter can then be defined with Concatenate (as Vizonex suggested) in order to remove the |
I can atleast use an overload for mypy to know that an instance method gets _LRUCacheWrapperInstanceMethod and another doesn't. But, still not sure how to get Concatenate into there without a separate class. |
@Vizonex Not sure if you want to take another attempt at it, but we're down to 10 errors in mypy. 8 of them are due to the instance issue, so if you can get Concatenate to work correctly you should be able to get it down to 2 errors (you can literally just run the command |
On Sun, Feb 2, 2025 at 4:30 PM Sam Bull ***@***.***> wrote:
@Vizonex <https://github.com/Vizonex> Not sure if you want to take
another attempt at it, but we're down to 10 errors in mypy. 8 of them are
due to the instance issue, so if you can get Concatenate to work correctly
you should be able to get it down to 2 errors.
—
Reply to this email directly, view it on GitHub
<#508 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/A3K7GGVMOZ7QQAYNSNTQOVD2N2L7PAVCNFSM6AAAAAA2XK7FTKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMMRZGU4DQNBRGQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
Sounds good.
|
Fixes #504.
There are still a couple of issues with this.
async-lru/async_lru/__init__.py
Lines 227 to 233 in 0f20348
partial()
use will cause new type errors. We can either wait until partial() is properly supported by typedshed/mypy (functools.partial should use PEP612 ParamSpec (depends on #8708) python/typeshed#8703 / functools.partial support python/mypy#1484), or we could expose aTypeAlias
of_LRUCacheWrapper
so it can be used in type annotations.There also seems to be an issue with pypy, possibly a bug in pypy, but needs further investigation.