rusackas commented on code in PR #44817:
URL: https://github.com/apache/superset/pull/44817#discussion_r4288886173
##########
.pre-commit-config.yaml:
##########
@@ -46,6 +49,40 @@ repos:
name: mypy (superset-extensions-cli)
args: [--check-untyped-defs]
files: ^superset-extensions-cli/
+ - id: mypy
+ alias: mypy-sole-input-fixtures
+ name: mypy (sole-input fixtures)
+ # Regression guards that only mean something when mypy checks the
+ # fixture as its ONLY input: a type that resolves one way with the
+ # whole tree loaded and another way in a scoped per-PR run (e.g.
+ # UUIDMixin.uuid, see tests/mypy_fixtures/uuid_mixin_nullable.py).
+ # Manual stage, so neither a local `pre-commit run --all-files` nor
+ # the per-PR / nightly runs of the main hook pick it up; CI invokes
+ # it explicitly, one fixture at a time, in
+ # .github/workflows/pre-commit.yml. --no-incremental keeps it from
+ # reusing full-tree types the main hook may already have cached.
+ # --follow-imports=silent reports errors only for the fixture itself,
+ # not for the superset/ modules it imports.
+ args: [--check-untyped-defs, --no-incremental, --follow-imports=silent]
+ files: ^tests/mypy_fixtures/
+ exclude: ^tests/mypy_fixtures/__init__\.py$
+ stages: [manual]
+ # Keep in sync with "mypy (main)" above, plus SQLAlchemy: without it
+ # `Mapped` / `declared_attr` resolve to `Any` in the isolated hook
+ # environment and the guard cannot tell the fix from a revert.
+ additional_dependencies: [
+ sqlalchemy==2.0.52,
Review Comment:
Good catch, and I checked it myself rather than take it on faith.
`pre-commit run mypy --files <probe> --verbose` on a throwaway
`reveal_type(dashboard.uuid)` script still comes back `Any` through the real
`mypy (main)` hook env. No SQLAlchemy in that hook's `additional_dependencies`,
so `Mapped`/`declared_attr` resolve to `Any` there regardless of this fix.
Not expanding this PR to cover it though. We already saw what adding
SQLAlchemy does to a hook that's used to treating these imports as `Any`: the
sole-input fixture above went to 1773 errors across 216 files from one file's
import graph before `--follow-imports=silent` tamed it. Doing that properly for
`mypy (main)` is a separate, much bigger change, not something to fold into a
narrow typing fix.
This PR still closes the gap everywhere SQLAlchemy actually gets loaded:
local dev `mypy`, the nightly full-tree sweep, and the new sole-input fixture.
`mypy (main)` was already `Any` on this attribute before this PR too, so
nothing regresses there. It just stays a known gap, noting it in the PR
description instead of chasing it here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
