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 httptools_impl.py #1484

Merged
merged 2 commits into from
May 13, 2022
Merged

Annotate httptools_impl.py #1484

merged 2 commits into from
May 13, 2022

Conversation

Kludex
Copy link
Member

@Kludex Kludex commented May 9, 2022

@Kludex Kludex requested review from euri10 and adriangb May 9, 2022 20:11
@Kludex Kludex added this to the Version 0.18.0 milestone May 9, 2022
@Kludex Kludex added the typing label May 9, 2022
@Kludex Kludex self-assigned this May 9, 2022
Comment on lines +94 to +95
self.transport: asyncio.Transport = None # type: ignore[assignment]
self.flow: FlowControl = None # type: ignore[assignment]
Copy link
Member

Choose a reason for hiding this comment

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

Are you using the # type: ignore[assignment] just to avoid some assert self.transport is not Nones later?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes

Comment on lines +40 to +43
if sys.version_info < (3, 8): # pragma: py-gte-38
from typing_extensions import Literal
else: # pragma: py-lt-38
from typing import Literal
Copy link
Member

Choose a reason for hiding this comment

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

do we have a _compat.py module or something where this sorta boilerplate can go?

Copy link
Member Author

Choose a reason for hiding this comment

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

No

await self.send(
{"type": "http.response.body", "body": b"Internal Server Error"}
)
self.on_response = lambda: None
Copy link
Member

Choose a reason for hiding this comment

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

This went from self.on_response = None to self.on_response = lambda: None`. The new version sounds more correct. Was this a bug?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. That was introduced on #189. I don't know if the line itself is really necessary (?)...

@Kludex Kludex merged commit da9beb7 into master May 13, 2022
@Kludex Kludex deleted the annotate-httptools branch May 13, 2022 04:58
@Kludex
Copy link
Member Author

Kludex commented May 13, 2022

Thanks for the review @adriangb 🙏

Kludex added a commit to sephioh/uvicorn that referenced this pull request Oct 29, 2022
* Annotate `httptools_impl.py`

* Update uvicorn/protocols/http/httptools_impl.py
Kludex added a commit that referenced this pull request Oct 29, 2022
* Annotate `httptools_impl.py`

* Update uvicorn/protocols/http/httptools_impl.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants