Skip to content

🎨 Fix typing annotation for semi-internal FastAPI.add_api_route()#10240

Merged
tiangolo merged 2 commits intofastapi:masterfrom
ordinary-jamie:bug/api-endpoint-typing-fix
Aug 17, 2024
Merged

🎨 Fix typing annotation for semi-internal FastAPI.add_api_route()#10240
tiangolo merged 2 commits intofastapi:masterfrom
ordinary-jamie:bug/api-endpoint-typing-fix

Conversation

@ordinary-jamie
Copy link
Copy Markdown
Contributor

@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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
Contributor Author

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

@danielfcollier
Copy link
Copy Markdown

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
@merlinz01
Copy link
Copy Markdown

@danielfcollier @Kludex Could we have this merged please?

@tiangolo tiangolo changed the title Fix typing for FastAPI add api route 🎨 Fix typing for FastAPI add_api_route Aug 17, 2024
@tiangolo tiangolo changed the title 🎨 Fix typing for FastAPI add_api_route 🎨 Fix typing annotation for semi-internal FastAPI.add_api_route() Aug 17, 2024
@tiangolo tiangolo added refactor and removed p4 labels Aug 17, 2024
@tiangolo
Copy link
Copy Markdown
Member

Awesome, thank you! 🚀 🍰

@tiangolo tiangolo enabled auto-merge (squash) August 17, 2024 04:51
@tiangolo tiangolo merged commit 659350e into fastapi:master Aug 17, 2024
@tiangolo tiangolo mentioned this pull request Aug 17, 2024
9 tasks
black-redoc pushed a commit to black-redoc/fastapi that referenced this pull request Aug 17, 2024
…astapi#10240)

Co-authored-by: Sebastián Ramírez <tiangolo@gmail.com>
black-redoc pushed a commit to black-redoc/fastapi that referenced this pull request Sep 12, 2024
…astapi#10240)

Co-authored-by: Sebastián Ramírez <tiangolo@gmail.com>
s-rigaud pushed a commit to s-rigaud/fastapi that referenced this pull request Jan 23, 2025
…astapi#10240)

Co-authored-by: Sebastián Ramírez <tiangolo@gmail.com>
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

6 participants