fix(typing): give Application.include_router a fully-known signature#181
Open
tmgbedu wants to merge 1 commit into
Open
fix(typing): give Application.include_router a fully-known signature#181tmgbedu wants to merge 1 commit into
tmgbedu wants to merge 1 commit into
Conversation
reportUnknownMemberType fired at every include_router call site because the method's **kwargs and return type were unannotated, so pyright/ basedpyright inferred an "Unknown"-tainted signature for it. The underlying FastAPI.include_router() itself is fully typed, so the leak was entirely in our own wrapper. Type the accepted kwargs with an IncludeRouterOptions TypedDict mirroring FastAPI's own include_router() parameters (mirrors the existing RouterOptions/RouteOptions pattern in fastapi/routers/router.py) and add an explicit "Application[TConfig]" return annotation. Types that require fastapi/starlette are referenced as quoted forward refs, consistent with the rest of this file, so importing fastapi_startkit.application still does not require the fastapi extra to be installed.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
tmgbedu
commented
Jul 16, 2026
tmgbedu
left a comment
Contributor
Author
There was a problem hiding this comment.
Code Review Verdict: APPROVE ✅ (posting as comment — GH blocks self-approval on own PR)
Reviewed for correctness, typing soundness, lazy-import contract, and regressions. All engineer claims verified independently.
Verification performed:
- TypedDict fields match FastAPI — all 9 fields (
prefix,tags,dependencies,responses,deprecated,include_in_schema,default_response_class,callbacks,generate_unique_id_function) matchFastAPI.include_router()in the installed FastAPI 0.136.3 exactly, correct types, no missing/extra fields. - Lazy-import contract preserved — empirically created the TypedDict +
Unpack[...]signature withfastapi/starletteimports blocked: succeeds,fastapinever enterssys.modules, FastAPI/Starlette-typed fields stay unevaluatedForwardRefs.params.Depends/Response/BaseRoute/APIRoutecorrectly quoted; only stdlib/Enumtypes unquoted. - No suppressions — no
# type: ignore/# pyright: ignore/# noqa. OnlyAnyis the legitimateDict[str, Any]mirroring FastAPI'sresponses. - CI checks hold — ruff check + format clean;
uv run pyright(basic mode) total unchanged at 659, application.py basic-mode errors identical main vs PR (all pre-existingNone-default /Optional[_fastapi], unrelated); pytest 1833 passed, 7 skipped (Postgres integration failures are environmental — fail identically on main, missing DB creds). CI pyright step iscontinue-on-error: true/ non-blocking. - Scope clean — only
application.pytouched (+23/-3), all new imports used, mirrors the existingRouterOptions/RouteOptionsTypedDict+Unpack idiom inrouter.py.
Not merging per instructions — PM to merge.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
reportUnknownMemberTypefired oninclude_routerbecauseApplication.include_router(fastapi_startkit/src/fastapi_startkit/application.py) took untyped**kwargsand had no return annotation, so pyright inferred a partially-Unknownsignature for every call site.FastAPI.include_router()itself is fully typed with explicit named parameters. The leak was entirely in our own wrapper method.include_routera complete, explicit signature: a newIncludeRouterOptionsTypedDict mirrors FastAPI's realinclude_router()keyword parameters (mirrors the existingRouterOptions/RouteOptionspattern already used infastapi/routers/router.py), consumed viaUnpack[IncludeRouterOptions], plus an explicit"Application[TConfig]"return type."APIRouter"/Type["BaseHTTPMiddleware"]style), sofastapi_startkit.applicationstill imports fine without thefastapiextra installed.Test plan
uv run --with basedpyright basedpyrightagainst astrictpyright config no longer reports anyinclude_routerdiagnostic inapplication.py(confirmed 0 matches, down from the reportedreportUnknownMemberType)uv run pyright(project's ownbasic-mode config used in CI) — error count unchanged (659, pre-existing unrelated baseline), zero errors inapplication.pybefore and afteruv run ruff check .anduv run ruff format --check— cleanuv run pytest tests/ -q --ignore=tests/masoniteorm/postgres— 1833 passed, 7 skipped