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

Add type hints to test_applications.py #2471

Merged
merged 8 commits into from
Feb 6, 2024

Conversation

TechNiick
Copy link
Contributor

Summary

Related to this
Type annotation added to test_applications.py

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

Comment on lines 26 to 31
from starlette.types import (
ASGIApp,
Receive,
Scope,
Send,
)
Copy link
Member

Choose a reason for hiding this comment

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

Please stop doing the multiline when not needed.

Suggested change
from starlette.types import (
ASGIApp,
Receive,
Scope,
Send,
)
from starlette.types import ASGIApp, Receive, Scope, Send

Choose a reason for hiding this comment

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

@Kludex You might want to add skip-magic-trailing-comma to the Ruff formatter config, it would collapse multiline statements that fit on one line such as this one.

Copy link
Member

@Kludex Kludex Feb 5, 2024

Choose a reason for hiding this comment

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

Seprate PR welcome.

Comment on lines 4 to 9
from typing import (
Any,
AsyncIterator,
Callable,
Generator,
)
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
from typing import (
Any,
AsyncIterator,
Callable,
Generator,
)
from typing import Any, AsyncIterator, Callable, Generator

Comment on lines 515 to 517
def test_middleware_stack_init(
test_client_factory: TestClientFactory,
) -> 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
def test_middleware_stack_init(
test_client_factory: TestClientFactory,
) -> None:
def test_middleware_stack_init(test_client_factory: TestClientFactory) -> None:

@@ -98,7 +103,7 @@ def custom_ws_exception_handler(websocket: WebSocket, exc: CustomWSException):
]
)

exception_handlers = {
exception_handlers: Any = {
Copy link
Member

Choose a reason for hiding this comment

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

Let's try to minimize the Anys. Can we remove this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If i remove any from exception_handlers: Any i get the following error:

error: Argument "exception_handlers" to "Starlette" has incompatible type "Dict[object, function]"; expected "Optional[Mapping[Any, Union[Callable[[Request, Exception], Union[Response, Awaitable[Response]]], Callable[[WebSocket, Exception], Awaitable[None]]]]]"

If I add exception_handlers: ExceptionHandlers i get the following errors:

Dict entry 0 has incompatible type "int": "Callable[[Request, HTTPException], Coroutine[Any, Any, JSONResponse]]"; expected "Any": "Union[Callable[[Request, Exception], Union[Response, Awaitable[Response]]], Callable[[WebSocket, Exception], Awaitable[None]]]"

Dict entry 1 has incompatible type "int": "Callable[[Request, HTTPException], Coroutine[Any, Any, JSONResponse]]"; expected "Any": "Union[Callable[[Request, Exception], Union[Response, Awaitable[Response]]], Callable[[WebSocket, Exception], Awaitable[None]]]"

Dict entry 2 has incompatible type "Type[HTTPException]": "Callable[[Request, HTTPException], Coroutine[Any, Any, JSONResponse]]"; expected "Any": "Union[Callable[[Request, Exception], Union[Response, Awaitable[Response]]], Callable[[WebSocket, Exception], Awaitable[None]]]"

Dict entry 3 has incompatible type "Type[CustomWSException]": "Callable[[WebSocket, CustomWSException], None]"; expected "Any": "Union[Callable[[Request, Exception], Union[Response, Awaitable[Response]]], Callable[[WebSocket, Exception], Awaitable[None]]]"

If I add exception_handlers: Mapping[Any, ExceptionHandler] as I saw in starlette/applications.py i get the same errors as above.

If I add exception_handlers: dict[ typing.Any, typing.Callable[[Request, Exception], Response] ] I get the following errors:

Dict entry 0 has incompatible type "int": "Callable[[Request, HTTPException], Coroutine[Any, Any, JSONResponse]]"; expected "Any": "Callable[[Request, Exception], Response]"

Dict entry 1 has incompatible type "int": "Callable[[Request, HTTPException], Coroutine[Any, Any, JSONResponse]]"; expected "Any": "Callable[[Request, Exception], Response]"

Dict entry 2 has incompatible type "Type[HTTPException]": "Callable[[Request, HTTPException], Coroutine[Any, Any, JSONResponse]]"; expected "Any": "Callable[[Request, Exception], Response]"

Dict entry 3 has incompatible type "Type[CustomWSException]": "Callable[[WebSocket, CustomWSException], None]"; expected "Any": "Callable[[Request, Exception], Response]"

I have searched everywhere related to exception_handlers type hints but this is all I could find and it looks like nothing is working. That's why I have added any there. Do you have any idea on how to fix this?

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer the type: ignore than Any.

@TechNiick TechNiick changed the title Added type annotations to test_applications.py Added type hints to test_applications.py Feb 4, 2024
@TechNiick TechNiick changed the title Added type hints to test_applications.py Add type hints to test_applications.py Feb 5, 2024
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.

Anys need to be removed.

startup_complete = False
cleanup_complete = False

@asynccontextmanager
async def lifespan(app):
async def lifespan(app: ASGIApp) -> Any:
Copy link
Member

Choose a reason for hiding this comment

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

remove the anys plz

Suggested change
async def lifespan(app: ASGIApp) -> Any:
async def lifespan(app: ASGIApp) -> Iterator[None]:

@TechNiick TechNiick requested a review from Kludex February 6, 2024 20:15
tests/test_applications.py Outdated Show resolved Hide resolved
@Kludex Kludex enabled auto-merge (squash) February 6, 2024 20:28
@Kludex Kludex merged commit 11b7ae7 into encode:master Feb 6, 2024
5 checks passed
@jonathanberthias jonathanberthias mentioned this pull request Feb 9, 2024
3 tasks
nixroxursox pushed a commit to nixroxursox/starlette that referenced this pull request Mar 18, 2024
* added type annotations to test_applications.py

* requested changes

* Apply suggestions from code review

* Apply suggestions from code review

* Update tests/test_applications.py

---------

Co-authored-by: Scirlat Danut <[email protected]>
Co-authored-by: Marcelo Trylesinski <[email protected]>
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