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

fix: mypy issues on importer.py and protocol/utils.py #991

Merged
merged 4 commits into from
Mar 11, 2021
Merged

fix: mypy issues on importer.py and protocol/utils.py #991

merged 4 commits into from
Mar 11, 2021

Conversation

Kludex
Copy link
Member

@Kludex Kludex commented Mar 8, 2021

Following https://github.com/django/asgiref/blob/main/asgiref/typing.py .

Fix mypy issues on the following files:

  • uvicorn/importer.py
  • uvicorn/protocols/utils.py

@Kludex Kludex changed the title fix: 2nd mypy issues fix: mypy issues on importer.py and protocol/utils.py Mar 9, 2021
@@ -29,3 +29,39 @@ class LifespanSendMessage(TypedDict):
"lifespan.shutdown.failed",
]
message: Optional[str]


class HTTPScope(TypedDict):
Copy link
Member

Choose a reason for hiding this comment

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

cant we import the types from asgiref directly ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would have to pip install from the latest asgiref branch, because that feature was not released yet. Do you prefer?



class ImportFromStringError(Exception):
pass


def import_from_string(import_str):
def import_from_string(import_str: Any) -> Any:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def import_from_string(import_str: Any) -> Any:
def import_from_string(import_str: Union[str, ModuleType]) -> ModuleType:

I've not tested it but it is what I would do, does this work ? Just asking if we can avoid Any here

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh! That's so cool. I didn't know about ModuleType... I'm going to try it later. Thank you! :)

Copy link
Member Author

Choose a reason for hiding this comment

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

It works. Applied.

@euri10
Copy link
Member

euri10 commented Mar 10, 2021 via email

@Kludex
Copy link
Member Author

Kludex commented Mar 10, 2021

@euri10 Done.

Should I create an issue to track those mypy PRs?

@Kludex Kludex mentioned this pull request Mar 10, 2021
2 tasks
@euri10
Copy link
Member

euri10 commented Mar 11, 2021

@euri10 Done.

Should I create an issue to track those mypy PRs?

no we good I think

@euri10 euri10 merged commit c5c7343 into encode:master Mar 11, 2021
@Kludex
Copy link
Member Author

Kludex commented Mar 11, 2021

I've already created (it helps to organize myself, but I can do it elsewhere), but if you think is just spam, I/you can close it. 😗 👍



class ImportFromStringError(Exception):
pass


def import_from_string(import_str):
def import_from_string(import_str: Union[ModuleType, str]) -> ModuleType:
Copy link
Contributor

Choose a reason for hiding this comment

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

@Kludex @euri10 This has raised 'ModuleType' object is not callable warnings all over (in my IDE - PyCharm).
For example:

self.loaded_app = self.loaded_app()

loop_setup()

  • Is this a valid issue?
  • Should we switch to Any as return type annotation?

Kludex added a commit to sephioh/uvicorn that referenced this pull request Oct 29, 2022
* fix some mypy issues

* fix some mypy issues

* add moduletype

* fix flake8 issue
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.

3 participants