Skip to content
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

Annotate run function #1421

Closed
wants to merge 3 commits into from
Closed

Annotate run function #1421

wants to merge 3 commits into from

Conversation

Kludex
Copy link
Sponsor Member

@Kludex Kludex commented Mar 27, 2022

The way I see it, we have two options here:

  1. Annotate the way I did.
  2. Unpack the kwargs, and add them to the config manually.

This PR is to improve the UX, as many users use uvicorn.run (me included).

This PR actually ignores the following mypy error:

  • uvicorn/main.py:445: error: Single overload definition, multiple required [misc]

Which means the ideal solution would be 2. So, any solution satisfies me, I'll let the reviewer decide the path to go here.

@euri10
Copy link
Member

euri10 commented Mar 27, 2022

hey @Kludex
I just tested it in pycharm and I dont see how it improves UX, care to explain ?
appart from that looks good to me, the CI just fails because of previous merge I think

@Kludex
Copy link
Sponsor Member Author

Kludex commented Mar 27, 2022

@euri10 autocomplete and docstring basically

Screencast.from.27-03-2022.20_12_36.mp4

) -> None:
"""Run uvicorn via Python code.

Args:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the docstring necessary here or the type hints are enough to get the completion ?

Copy link
Sponsor Member Author

@Kludex Kludex Mar 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Annotation is enough for that, the docstring are a plus

@euri10
Copy link
Member

euri10 commented Mar 27, 2022

ok no comments appart the one above, this does not make the autocomplete work on pycharm though

@Kludex
Copy link
Sponsor Member Author

Kludex commented Mar 28, 2022

If it doesn't, then it's not a valid solution :/

@Kludex Kludex mentioned this pull request Mar 28, 2022
@euri10
Copy link
Member

euri10 commented Mar 28, 2022

ok let's close this #1423 is working as intended

@euri10 euri10 closed this Mar 28, 2022
@Kludex Kludex deleted the annotate/run branch October 2, 2022 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants