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

Typing/added typing to httptools_impl.py module #1014

Conversation

jkklapp
Copy link
Contributor

@jkklapp jkklapp commented Apr 5, 2021

Part of #998

@jkklapp jkklapp changed the title Typing/added typing to httptools impl module Typing/added typing to httptools_impl.py module Apr 5, 2021
@jkklapp
Copy link
Contributor Author

jkklapp commented Apr 5, 2021

wip

@jkklapp
Copy link
Contributor Author

jkklapp commented Apr 5, 2021

@Kludex I added some type: ignore lines after spending a bit with each. It would be good to have some help from you or others to try and fix them though.

@jkklapp
Copy link
Contributor Author

jkklapp commented Apr 10, 2021

@Kludex bump :)

Copy link
Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

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

There are too many ignores, can you check those? I didn't comment on all of them. But I believe some of those are necessary because this file needs some others to be completed properly, am I right?

if self.write_paused:
self.write_paused = False
self._is_writable_event.set()


async def service_unavailable(scope, receive, send):
async def service_unavailable(scope: Scope, receive: Callable, send: Callable) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

The idea is to follow asgiref. One of my PRs already have the types for receive and send.

self,
config: Config,
server_state: ServerState,
_loop: asyncio.AbstractEventLoop = None,
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
_loop: asyncio.AbstractEventLoop = None,
_loop: Optional[asyncio.AbstractEventLoop] = None,

self.server: Optional[Tuple[str, int]] = None
self.client: Optional[Tuple[str, int]] = None
self.scheme: str
self.pipeline: list = []
Copy link
Member

Choose a reason for hiding this comment

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

Too generic.

self.expect_100_continue = False
self.cycle = None
self.cycle: RequestResponseCycle = None # type: ignore
Copy link
Member

Choose a reason for hiding this comment

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

Why ignoring?

Suggested change
self.cycle: RequestResponseCycle = None # type: ignore
self.cycle: Optional[RequestResponseCycle] = None # type: ignore

self.headers = None
self.url: Optional[str] = None
self.scope: dict = {}
self.headers: list = []
Copy link
Member

Choose a reason for hiding this comment

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

Is it List[Tuple[str]]?


# Protocol interface
def connection_made(self, transport):
def connection_made(self, transport: asyncio.Transport) -> None: # type: ignore
Copy link
Member

Choose a reason for hiding this comment

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

Why ignoring?

@@ -262,7 +272,7 @@ def on_headers_complete(self):

existing_cycle = self.cycle
self.cycle = RequestResponseCycle(
scope=self.scope,
scope=self.scope, # type: ignore
Copy link
Member

Choose a reason for hiding this comment

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

What's the issue?

self.expected_content_length = 0

# ASGI exception wrapper
async def run_asgi(self, app):
async def run_asgi(self, app: Callable) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

Too generic. One of my PRs have this already, or even the current one already has? Not sure.

@@ -435,7 +445,7 @@ async def send_500_response(self):
)

# ASGI interface
async def send(self, message):
async def send(self, message: dict) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

asgiref. One of my PRs already has the Message type.

@@ -544,7 +554,7 @@ async def send(self, message):
msg = "Unexpected ASGI message '%s' sent, after response already completed."
raise RuntimeError(msg % message_type)

async def receive(self):
async def receive(self) -> Dict:
Copy link
Member

Choose a reason for hiding this comment

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

This is Message as well.

@Kludex Kludex mentioned this pull request Apr 10, 2021
2 tasks
@jkklapp
Copy link
Contributor Author

jkklapp commented Apr 10, 2021

@Kludex I have been checking your PRs to add the correct types however Im getting mypy issues I am struggling to solve. Hence I added more lines with type: ignore :(

Is it useful if I paste some mypy outputs here?

@Kludex
Copy link
Member

Kludex commented Apr 10, 2021

I don't know... Probably not to be honest. But we can try.

I've tried to crack this file before, but I felt like it depends on other stuff to be merged before 😞

@Kludex
Copy link
Member

Kludex commented May 29, 2021

@jkklapp We're now more active on those PRs.

We've choose to make use of asgiref typing module instead of the _types.py (which was removed). Do you mind updating this PR?

@Kludex
Copy link
Member

Kludex commented Jun 29, 2021

@jkklapp Thanks for the efforts! I'll take the lead now. 😄

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.

2 participants