Mark every checkable override with @override - #918
Conversation
| warn_unreachable = true | ||
| warn_unused_configs = true | ||
| # Overrides are marked with pyathena.util.override (PEP 698). | ||
| enable_error_code = ["explicit-override"] |
There was a problem hiding this comment.
Self-review round 1 (implementation behavior): CLEAN
Base 39f22dc1d711bdd460adbbfd1736d0bff347ec80, head 3a134a9d7712621f09b8df98fe1ae3211b386016. All 56 changed files covered.
Covered:
- Runtime behavior:
pyathena.util.overridereturns the function unchanged, so the decorator order (below@property/@classmethod/@staticmethod, above@reflection.cache) does not change any descriptor.AthenaStruct._static_cache_keyand_ArrayUpdate._from_objectsstay plain properties, as on master. The base attributes are SQLAlchemy's_memoized_propertyand_non_memoized_property, which is unchanged. - Import cycles: each of the 55 changed
pyathenamodules imports on its own in a fresh interpreter.import pyathenanow loadspyathena.util. That works because the package bindsDataErrorfrompyathena.errorbefore importingpyathena.util, which importsDataErrorfrom the package. - SQLAlchemy test plugin (
setup.cfgrequirement_cls): all 39Requirementsproperties give the same exclusion types andenabledvalues as on master. - Framework contracts: no dialect, reflection, or fsspec code path changes. The fsspec overrides are not touched.
- Type checking:
explicit-overridereports a removed marker (visit_FLOAT), and a renamed base method is reported at its 12 marked overrides. The only remaining unused ignores are the three that already exist on master. - Tests: no new tests. No behavior changes, and the guard is mypy itself, which
just lintruns.
Findings: none.
Limitations: the AWS suites have not run yet. They run when the PR is marked Ready.
Mark the methods that override a base class method with pyathena.util.override, and enable mypy's explicit-override error code so that mypy reports an override without the decorator. This covers the synchronous and asyncio cursors and result sets, converters, readers and the formatter, the SQLAlchemy dialects, compilers, types and test-suite requirements, and the filesystem helpers whose bases are typed. mypy skips both checks for an unannotated function: it does not report a marked method whose base method is missing, and it does not report an unannotated property override without the decorator. 79 of the marked methods are unannotated, mostly SQLAlchemy overrides. The fsspec overrides stay unmarked: fsspec has no type information, so mypy rejects @OverRide on them. mypy cannot accept @OverRide on a property that overrides a property with a setter (python/mypy#15900); those seven getters keep the decorator and ignore the error. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
3a134a9 to
868baf5
Compare
|
|
||
| class Requirements(SuiteRequirements): | ||
| @property | ||
| @override |
There was a problem hiding this comment.
Self-review round 2 (claims, callers, evidence): FINDINGS (repaired)
Base 39f22dc1d711bdd460adbbfd1736d0bff347ec80, head 3a134a9d7712621f09b8df98fe1ae3211b386016 → 868baf5b985a9972f1cd9d688ac01a9cd6e566a0 (same tree, commit message only).
Finding: the PR body said mypy can still check these 41 SQLAlchemy overrides. The commit message said explicit-override reports every override without the decorator. Both are false for unannotated functions. Here, removing @override from comment_reflection, or renaming it or AthenaStruct._static_cache_key, passes mypy. A minimal case reproduces it: mypy 1.14.0 skips the "no base method" check for an unannotated method, and skips explicit-override for an unannotated property. 79 of the 297 markers are on unannotated functions (39 here, 14 in array.py, 12 in compiler.py, and others).
Repair: narrowed the commit message and the PR body, and listed the limits. The markers stay, because they still record the override for readers and for ruff's ignore-decorators. Annotating these methods is out of scope.
Other claims checked:
- Counts: 44 markers on master + 253 = 297 decorator lines. mypy reported 212 sites (215 output lines include 3 notes).
- fsspec 2026.9.0 has no
py.typed. - mypy 1.14.0 is the locked version.
explicit-overridefalse positive when overriding property with setter python/mypy#15900 was opened on 2023-08-18 and is still open. The versions and decorator placements listed were all tried. - The category list was incomplete (formatter, the asyncio
get_default_converter,AioCursor.arraysize,AioSparkCursor.calculation_execution). Fixed. - Existing callers:
mypy .passes in a separate Python 3.11 venv, and with--python-version3.12, 3.13 and 3.14.pyathenaand the SQLAlchemy modules import under 3.11. - Documentation: no docs mention
@overrideyet. The policy goes intodocs/contributing.mdin the ruff PR. - AWS operator: not applicable. No runtime or AWS request changes.
|
|
||
| from pyathena.error import * # noqa: F403 | ||
| from pyathena.options import ExecuteOptions as ExecuteOptions | ||
| from pyathena.util import override |
There was a problem hiding this comment.
Independent review (relayed): CLEAN
Reviewer: Codex CLI 0.159.3, model gpt-6-astra, reasoning effort high, sandbox read-only, session 01a0f874-9425-7850-9f24-b962e38f4a38. It did not author the change.
Scope: base 39f22dc1d711bdd460adbbfd1736d0bff347ec80, head 868baf5b985a9972f1cd9d688ac01a9cd6e566a0. The reviewer read a detached clean checkout of the head and the literal diff. Its prompt left out the PR number, description, commit message and earlier review findings. Constraints: no edits, builds, tests, type checkers, network, GitHub or memory. This is a static review.
Reviewer's result:
Surfaces covered: Full diff: 55 Python files and
pyproject.toml. Runtime identity decorator, imports, descriptors, and decorator ordering. PyAthena inheritance across cursors, result sets, converters, readers, Spark, and filesystem helpers; fsspec overrides excluded. SQLAlchemy dialects, compilers, types,_HashableDict, and the local compliance-plugin loading path. All seven property suppressions and the mypy configuration comment.Verdict: CLEAN
No actionable defect found in the inspected sources.
DataErroris exported beforepyathena.utilimports it back, so that dependency cycle does not access an undefined name. Decorator ordering preserves descriptors and cached wrappers. Removing the__hash__type-ignore has no runtime effect. No missing markers were found along the traced PyAthena inheritance paths.Coverage limitation: SQLAlchemy's upstream sources and mypy's implementation/issue text are absent from this checkout. Consequently, upstream override targets and the precise necessity of the suppressions remain unverified.
Author note on the stated limitation: the snapshot does not contain the SQLAlchemy and mypy sources. The author's evidence covers that gap. The 212 explicit-override sites are the overrides mypy itself reported, with the base class named. The 41 unannotated SQLAlchemy sites came from a runtime MRO scan that found the base attribute. The need for the seven suppressions was reproduced with a minimal example on mypy 1.14.0 through 2.3.1. The review snapshot and the PR worktree were unchanged afterward.
WHAT
pyathena.util.override(added in Mark the asyncio overrides of sync methods with @override #917), for 297 in total:explicit-overrideerror code reports. They are in the synchronous cursors and result sets, a few asyncio cursor members (get_default_converter,AioCursor.arraysize,AioSparkCursor.calculation_execution), the converters, readers and formatter, the SQLAlchemy dialects, compilers (visit_*) and types,filesystem/s3_object.pyands3_executor.py, andDBAPITypeObject.__eq__/__ne__/__hash__.SuiteRequirementsproperties insqlalchemy/requirements.py,_ArrayUpdate._from_objectsandAthenaStruct._static_cache_key. These were found by checking every class's MRO at runtime.explicit-overridein[tool.mypy], so mypy reports an override without the decorator.@overridegoes below@property,@classmethodand@staticmethod, and above@reflection.cache.@overridemakes the# type: ignore[override]on_HashableDict.__hash__unused, so the comment is removed.filesystem/s3.pyands3_async.py. fsspec has no type information (nopy.typedin 2026.9.0), so mypy rejects@overridethere ("no base method was found").@overridebut need# type: ignore[explicit-override]:arraysizeon six cursors andAthenaDDLCompiler.preparer. mypy reports a property that overrides a property with a setter even when it is marked (python/mypy#15900, open since 2023-08). This happens with every version tried: 1.14.0 (locked), 1.15.0, 1.16.1, 1.18.2 and 2.3.1. Marking the getter, the setter, both, or placing the decorator above@propertydoes not help.Limits of the mypy checks:
explicit-overridedoes not report an unannotated property override without the decorator. 79 of the 297 marked methods are unannotated: the 41 above, 14 insqlalchemy/array.py, 12 insqlalchemy/compiler.py, and a few dialect methods. For these,@overrideonly records the override, for readers and for ruff.pyathena.util.overridereturns the method unchanged at runtime.import pyathenanow importspyathena.utilbecauseDBAPITypeObjectuses it. This works because the package importspyathena.errorbeforepyathena.util, which importsDataErrorfrom the package.WHY
Part of #882 (step 1 of the agreed plan).
#882 exempts overrides from the docstring requirement when they behave like the parent method, by marking them with
@override. ruff's pydocstyle rules skip functions with a decorator listed inignore-decorators, and the next PR enables those rules withignore-decorators = ["pyathena.util.override"]. With the methods in this PR marked, ruff's D102 findings inpyathena/drop from 528 to 367 under that configuration.explicit-overridereports new annotated overrides that lack the marker. For annotated methods, mypy also reports a marked method whose base method has been renamed or removed, which #916 introduced for the asyncio overrides.TEST
Tested commit: 868baf5 (same tree as 3a134a9; only the commit message changed)
just lint: passed (ruff, ruff format, mypy withexplicit-override, cfn-lint, license headers).mypy .in a separate Python 3.11 venv: passed.mypy --python-version 3.12/3.13/3.14in the 3.13 venv: passed.uv run mypy --warn-unused-ignores .: only the three unused ignores that already exist on master remain (sqlalchemy/compiler.py,sqlalchemy/base.py×2).BaseCursor.get_default_converterreports the 12 marked overrides ("marked as an override, but no base method was found").@overridefromAthenaTypeCompiler.visit_FLOATreportsexplicit-override.@overridefromRequirements.comment_reflection, or renaming it orAthenaStruct._static_cache_key, is not reported (unannotated; see the limits above).pyathenamodules import together, and each of the 55 changed modules imports in a fresh interpreter (also under Python 3.11 forpyathenaand the SQLAlchemy modules).override(f) is f. The markedarraysizeandpreparerare still properties, andarraysizekeeps its setter. All 39Requirements()properties give the same exclusion types andenabledvalues as on master.AioBaseCursor._pollis still a coroutine function.pytest --noconftestwith dummy environment variables) for the converter, formatter, model, options, parser, util, arrow/pandas/polars converter and util, and SQLAlchemy type and compiler modules: 403 passed. The 78 errors come from tests that need theformatterandcursorfixtures from conftest.sphinx-build -b html docsbefore and after: same warnings. The rendered API pages are the same apart from a timestamp default value.🤖 Generated with Claude Code