-
-
Notifications
You must be signed in to change notification settings - Fork 672
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
Added async_command to typer.Typer #332
Conversation
But shouldn't we allow other async runtimes other than asyncio? Not sure how that would work, though. Maybe adding anyio as an optional dependency. |
@cauebs Certainly, I'm somewhat new to async runtimes so I just assumed that asyncio was the bultin and thus "only" one. We can use anyio. For some reason when I do the method injection as I do in the issue it works. But this PR when I test it with the following test function gives me the following: import typer
from asyncio import sleep
app=typer.Typer()
# The command we want to be accessible by both
# the async library and the CLI
@app.async_command()
async def foo():
"""Foo bar"""
return await sleep(5)
if __name__=="__main__":
app()
|
Actually now that error isn't reproducing. My current version seems to work. |
typer/main.py
Outdated
if backend is not None: | ||
from anyio import run | ||
else: | ||
from asyncio import run |
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.
Why not use a try...except ImportError
instead? That way you don't need the backend
arg.
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.
Well I thought about that but what if you need to use the backend arg? Is the backend arg automatic?
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.
What do you think actually about this solution:
Instead of adding anyio or any other tool to extras, what if we just had run_func
as a kwarg
and default it to asyncio.run
(as that is builtin). So that anyone can provide literally any tool with any arguments they want. Ultimately they have to import it themselves which keeps it out of our dependency files, making installation easier.
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.
iirc, asyncio.run
has a different signature to anyio.run
and trio.run
, e.g.:
async def foo(bar): ...
asyncio.run(foo(bar))
anyio.run(foo, bar)
trio.run(foo, bar)
But there might be a way to handle that elegantly.
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.
asyncio: run_func = lambda f, *a, **k: asyncio.run(f(*a, **k))
anyio: run_func = lambda f, *a, **k: anyio.run(f, *a, **k)
trio: run_func = lambda f, *a, **k: trio.run(f, *a, **k)
And we just make our run_func take lambda f, *a, **k
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.
Implemented a demo pending review and feedback.
typer/main.py
Outdated
self, | ||
name: Optional[str] = None, | ||
*, | ||
run_func: RunFunction = lambda f, *args, **kwargs: asyncio.run(f(*args, **kwargs)), |
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'd suggest to add this argument to the constructor as well. From my point of view it is more likely that you only overwrite the run function once (depending on your async runtime) or in rare special cases.
Maybe do something like
def async_command(
# ...
run_func: Optional[RunFunction] = None
# ...
):
# ...
run_func = run_func or self.run_func
# ...
I don't think it is needed to extend the API with another function |
📝 Docs preview for commit 2a255b2 at: https://639cea0e2c118d099d47fb93--typertiangolo.netlify.app |
From my comment in:
tiangolo#88
Allows command to be used with async.
Let me know if using *args, **kwargs is ok or if it needs to be typed. *args, **kwargs will be more maintainable. Further we could just make @app.command DETECT if it's attached to an async function, but that might be more challenging.