fix: update the async transactional types to not require extra awaits #1045
Conversation
product-auto-label
bot
added
size: s
jekozyra
force-pushed
the
jillian/update-types-for-async-transactions
branch
from
b216867 to
ca23405
Compare
|
@wu-hui could I get a review here please? |
| @@ -41,6 +52,10 @@ | |||
| from google.cloud.firestore_v1.query_profile import ExplainOptions | |||
|
|
|||
|
|
|||
| T = TypeVar("T") | |||
| P = ParamSpec("P") | |||
There was a problem hiding this comment.
It looks like ParamSpec was added in python 3.10, and we need to support 3.8
There was a problem hiding this comment.
I added typing_extensions to support expanded type hinting for older python versions. Let me know if that works, otherwise I'll come up with something more hacky for the signatures.
There was a problem hiding this comment.
Sorry, adding an extra dependency to setup.py isn't an option unfortunately
Let me know if you're able to experiment with getting this working, or I can try to take it from here if you don't have the capacity
Thanks!
There was a problem hiding this comment.
I updated it to use a protocol instead. The type checking still looks to achieve the intended result on my end.
There was a problem hiding this comment.
Sorry, it looks like I was wrong when I said we support 3.8+. We actually still support 3.7 here, which doesn't support Protocol either.
If we can keep some of this confined to the TYPE_CHECKING block, that could be a solution, since we our mypy checks run against a more recent version of Python (Maybe ParamSpec could work that way?). Or maybe there's another way to accomplish the same thing
If you're interested in running some of these tests locally, there's a short section in the CONTRIBUTING file on using nox. It works well with pyenv. Or you can let me know if you want me to take more of this on
I'll take another look at this next week, thanks!
There was a problem hiding this comment.
I'm not going to have time to continue looking at this for the forseeable future.
jekozyra
force-pushed
the
jillian/update-types-for-async-transactions
branch
from
69ff68b to
576a8a8
Compare
|
Fixed in #1066 |
I updated the types of a few things in async_transactional to ensure that when a function wrapped with
@async_transactionalis called:the return type of the inner function is correctly detected as the correct final result by the typechecker
we can pass additional arguments to the wrapped function without the typechecker complaining
Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
Ensure the tests and linter pass
Code coverage does not decrease (if any source code was changed)
Appropriate docs were updated (if necessary)
Fixes #1044 🦕