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 typing for FastAPI add api route #10240

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ordinary-jamie
Copy link

@ordinary-jamie ordinary-jamie commented Sep 12, 2023

Closes #10236

Make typing of endpoint parameter consistent with downstream calls and other utilities.

Justification for picking Callable[..., Any] over original:

  • FastAPI.add_api_route makes a direct call to APIRouter.add_api_route anyways
  • fastapi.dependencies.utils.get_typed_return_annotation has signature def get_typed_return_annotation(call: Callable[..., Any]) -> Any:
  • A lot of other functions with similar parameters use the same typing (e.g. FastAPI.add_api_websocket_route)
  • The api router instantiates route_class: Type[APIRoute] of which fastapi.routing.APIRoute subclasses starlette.routing.Route which also types endpoint with Callable[..., typing.Any]

Make typing of endpoint parameter consistent with downstream calls and
other utilities.
Copy link
Contributor

@iudeen iudeen left a comment

Choose a reason for hiding this comment

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

I like the justification.

@danielfcollier
Copy link

danielfcollier commented Sep 15, 2023

As far as I've tested, this PR is missing the test coverage for issue #10236. Checked this modification with the tests and modification made on PR #10250, but this modification would be breaking the tests cases (I've also tested for the generators case, since this modification could be more general).

@ordinary-jamie
Copy link
Author

Thanks @danielfcollier, can you send a link to the CI build / enumerate the tests that failed?

@danielfcollier
Copy link

danielfcollier commented Sep 18, 2023

Hi @ordinary-jamie , I run locally. But, I've just put the tests on PR #10258 to show you what would be required to add coverage for Issue #10236.

There is still a more complex problem with generators, but I've left the tests for the generator's case commented.

@alejsdev alejsdev added the p4 label Jan 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent add_api_route types
4 participants