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 *args to Middleware and improve its type hints #2381

Merged

Conversation

pawelrubin
Copy link
Contributor

Use ParamSpec to provide concrete type annotations for middleware's parameters.

Summary

See #2380

Checklist

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

starlette/applications.py Show resolved Hide resolved
Use ParamSpec to provide concrete type annotations for middleware's parameters.
@pawelrubin pawelrubin force-pushed the feat/improve-type-annotations-for-middleware branch from 0f4c8c5 to 162440c Compare December 18, 2023 15:56
@pawelrubin

This comment was marked as outdated.

@pawelrubin pawelrubin force-pushed the feat/improve-type-annotations-for-middleware branch from 3819c8e to d22a612 Compare December 18, 2023 16:30
starlette/applications.py Outdated Show resolved Hide resolved
@pawelrubin pawelrubin force-pushed the feat/improve-type-annotations-for-middleware branch from 69bee8d to 0d36db7 Compare December 20, 2023 10:52
@Kludex Kludex changed the title feat: Improve Middleware type annotations. Add *args to Middleware and its type hints Dec 20, 2023
@Kludex Kludex changed the title Add *args to Middleware and its type hints Add *args to Middleware and improve its type hints Dec 20, 2023
def add_middleware(self, middleware_class: type, **options: typing.Any) -> None:
def add_middleware(
self,
middleware_class: typing.Type[MiddlewareClass[P]],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figured that we could achieve more precise annotation via typing.Protocol. Moreover, this ensures that the middleware instance does adhere to the ASGI protocol.

Copy link
Member

@adriangb adriangb left a comment

Choose a reason for hiding this comment

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

Looks good to me other than a slight preference for not adding new public symbols

P = ParamSpec("P")


class MiddlewareClass(Protocol[P]): # pragma: no cover
Copy link
Member

Choose a reason for hiding this comment

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

Can we somehow make this private?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

Copy link
Member

Choose a reason for hiding this comment

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

Also why is there pragma here? Doesn't it get run just by being imported?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This particular line - as well as the class body - does get run when imported. However, the empty body (...) of __init__ and __call__ does not, since those are not being called. Putting pragma at the top of the class was merely a shortcut - not to put pragma in both of those functions.

Though it's probably best to be explicit. Changed.

Copy link
Member

Choose a reason for hiding this comment

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

Makes total sense! It just confused me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! :)

@pawelrubin pawelrubin force-pushed the feat/improve-type-annotations-for-middleware branch from 7b26974 to 5680abb Compare December 20, 2023 18:02
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.

None yet

3 participants