-
-
Notifications
You must be signed in to change notification settings - Fork 27
Add py314 to test matrix #467
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
Conversation
3041a89 to
43cd5a7
Compare
43cd5a7 to
003e45d
Compare
| return asyncio.iscoroutinefunction(obj) or ( | ||
| callable(obj) and asyncio.iscoroutinefunction(obj.__call__) | ||
| return inspect.iscoroutinefunction(obj) or ( | ||
| callable(obj) and inspect.iscoroutinefunction(getattr(obj, "__call__", None)) |
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.
I'm sorry to ask a dummy question, is the getattr necessary here?
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.
prob. not, unsure what i was thinking there.
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.
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.
Sorry for the back and forth, i've been writing loads of tests to dig here see.
But the gist of it; Yes, getattr is needed. inspect.iscoroutinefunction(obj) only works for actual async function objects. Instances with async call return False there, so you must look at obj.call. Using getattr(obj, "call", None) avoids an AttributeError and cleanly checks the bound method. Without it you’d misclassify async callable instances or need a try/except.
EDIT;
Make sure to check look at these test cases; https://github.com/janbjorge/pgqueuer/pull/473/files#diff-998ed263122511bfcb910e08f8830fd62a83287a5b2aec5a40e351f5ba5d47adR536
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.
thanks for your reply, I didn't aware of that
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.
thanks for asking @trim21 it lead me down an crazy rabithole that i enjoyed!
The below PR / issues needs to be closed before we can add support for py314.