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

Fix exception_handlers type hints #1360

Merged
merged 3 commits into from
Dec 22, 2021
Merged

Conversation

aminalaee
Copy link
Member

@aminalaee aminalaee commented Dec 14, 2021

Closes #1142 and replaces #1139.

Changes exception_handlers type hints:

typing.Mapping[
    typing.Any, typing.Callable[[Request, Exception], Response]
]

I also changed the callable type hints, I think it makes sense to be more explicit, this will catch user-defined callable mismatches (?).

As mentioned in #1139 we can't use typing.Mapping[typing.Union[int, ..], ...] since dict keys are not covariant.I think we could use typing.TypedDict but only after 3.8.

change type hints in ExceptionMiddleware
@aminalaee aminalaee force-pushed the fix-typehint-of-exception-handlers branch from b6cf47e to 42902fb Compare December 14, 2021 09:40
@aminalaee aminalaee added the typing Type annotations or mypy issues label Dec 14, 2021
app: ASGIApp,
handlers: typing.Mapping[
typing.Any, typing.Callable[[Request, Exception], Response]
] = None,
Copy link

Choose a reason for hiding this comment

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

It's quite a complicated type, should we move it to starlette.types? Like ExceptionHandler type

Copy link
Member

Choose a reason for hiding this comment

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

The current types on types.py are going to be removed at some point: #1217

That file will only contain this type at some point. Is that a problem? 🤔

Copy link

Choose a reason for hiding this comment

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

Didn't notice the Issue,
This mapping type just seems a bit complicated to me to be copied multiple times

Copy link
Member Author

Choose a reason for hiding this comment

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

One drawback I noticed in VScode, haven't really checked and there might be some workarounds, is that they don't show function signatures when using the aliases as you mention. Not sure if it happens on PyCharm and other tools.

@k4black
Copy link

k4black commented Dec 14, 2021

Speaking about typing.TypedDict you can add python version check as a nice way to show that you'll migrate to dict when 3.7 will be deprecated

@Kludex Kludex mentioned this pull request Dec 20, 2021
8 tasks
@antonagestam
Copy link

@aminalaee @k4black FYI, I've opened a PR that improves on the typing added in this PR: #1456

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
typing Type annotations or mypy issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mypy complains about exception_handlers dict with HTTPException key
4 participants