-
Notifications
You must be signed in to change notification settings - Fork 96
Fix all typing issues #134
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
base: master
Are you sure you want to change the base?
Conversation
- on_message_begin() | ||
- on_url(url: bytes) | ||
- on_header(name: bytes, value: bytes) | ||
- on_headers_complete() | ||
- on_body(body: bytes) | ||
- on_message_complete() | ||
- on_chunk_header() | ||
- on_chunk_complete() | ||
- on_status(status: bytes) |
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.
This is not needed. Implementers can follow the protocol.
- on_chunk_header() | ||
- on_chunk_complete() | ||
- on_status(status: bytes) | ||
def __init__(self, protocol: HTTPProtocol | object) -> None: |
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.
object
is less permissive than Any
.
Python still doesn't support "not required class method protocol": https://discuss.python.org/t/discussion-optional-class-and-protocol-fields-and-methods
protocol (HTTPProtocol): Callback interface for the parser. | ||
""" | ||
|
||
def set_dangerous_leniencies( |
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.
This was missing.
httptools/parser/parser.pyi
Outdated
... | ||
|
||
def feed_data(self, data: Union[bytes, bytearray, memoryview, array]) -> None: | ||
def feed_data(self, data: Union[bytes, bytearray, memoryview, array[int]]) -> None: |
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.
array
is a Generic
.
def on_message_begin(self) -> None: ... | ||
def on_url(self, url: bytes) -> None: ... | ||
def on_header(self, name: bytes, value: bytes) -> None: ... | ||
def on_headers_complete(self) -> None: ... | ||
def on_body(self, body: bytes) -> None: ... | ||
def on_message_complete(self) -> None: ... | ||
def on_chunk_header(self) -> None: ... | ||
def on_chunk_complete(self) -> None: ... | ||
def on_status(self, status: bytes) -> None: ... |
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.
Missing self
.
Returns an instance of ``httptools.URL`` class with the | ||
following attributes: | ||
- schema: bytes | ||
- host: bytes | ||
- port: int | ||
- path: bytes | ||
- query: bytes | ||
- fragment: bytes | ||
- userinfo: bytes |
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.
Irrelevant. The URL
is defined above.
__all__ = ( | ||
# protocol | ||
"HTTPProtocol", | ||
# parser | ||
"HttpParser", | ||
"HttpRequestParser", | ||
"HttpResponseParser", | ||
# errors | ||
"HttpParserError", | ||
"HttpParserCallbackError", | ||
"HttpParserInvalidStatusError", | ||
"HttpParserInvalidMethodError", | ||
"HttpParserInvalidURLError", | ||
"HttpParserUpgrade", | ||
# url_parser | ||
"parse_url", | ||
) |
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.
Proper definition.
I've dropped the |
I've tested this on uvicorn, it works like a charm. It would be nice to add a type checker in this repository. EDIT: I meant the typing... 😅 |
Thank you! Looks there's an error: Let me add a CI workflow to check typing. |
Then it seems the issue is that the I'm on a plane. I'll fix this tomorrow. |
It seems I've added the type checker to the pipeline, but feel free to tweak whatever you want. |
@fantix can you please check this?