Skip to content

Mark every checkable override with @override - #918

Merged
laughingman7743 merged 1 commit into
masterfrom
chore/882-override-all
Oct 2, 2026
Merged

laughingman7743 merged 1 commit into
masterfrom
chore/882-override-all

Conversation

@laughingman7743

@laughingman7743 laughingman7743 commented Oct 1, 2026 •

Copy link
Copy Markdown
Member

WHAT

  • Mark 253 more overriding methods with pyathena.util.override (added in Mark the asyncio overrides of sync methods with @override #917), for 297 in total:
    • 212 that mypy's explicit-override error 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.py and s3_executor.py, and DBAPITypeObject.__eq__/__ne__/__hash__.
    • 41 unannotated SQLAlchemy property overrides that the error code does not report: the 39 SuiteRequirements properties in sqlalchemy/requirements.py, _ArrayUpdate._from_objects and AthenaStruct._static_cache_key. These were found by checking every class's MRO at runtime.
  • Enable explicit-override in [tool.mypy], so mypy reports an override without the decorator.
  • Decorator order follows PEP 698: @override goes below @property, @classmethod and @staticmethod, and above @reflection.cache.
  • @override makes the # type: ignore[override] on _HashableDict.__hash__ unused, so the comment is removed.
  • Not marked:
    • The 48 fsspec overrides in filesystem/s3.py and s3_async.py. fsspec has no type information (no py.typed in 2026.9.0), so mypy rejects @override there ("no base method was found").
    • Seven getters keep @override but need # type: ignore[explicit-override]: arraysize on six cursors and AthenaDDLCompiler.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 @property does not help.

Limits of the mypy checks:

  • mypy skips both checks for an unannotated function. It does not report a marked method whose base method is missing, and explicit-override does not report an unannotated property override without the decorator. 79 of the 297 marked methods are unannotated: the 41 above, 14 in sqlalchemy/array.py, 12 in sqlalchemy/compiler.py, and a few dialect methods. For these, @override only records the override, for readers and for ruff.
  • As before, fsspec overrides are not checked.

pyathena.util.override returns the method unchanged at runtime. import pyathena now imports pyathena.util because DBAPITypeObject uses it. This works because the package imports pyathena.error before pyathena.util, which imports DataError from 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 in ignore-decorators, and the next PR enables those rules with ignore-decorators = ["pyathena.util.override"]. With the methods in this PR marked, ruff's D102 findings in pyathena/ drop from 528 to 367 under that configuration. explicit-override reports 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 with explicit-override, cfn-lint, license headers).
  • mypy . in a separate Python 3.11 venv: passed. mypy --python-version 3.12/3.13/3.14 in 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).
  • Mutation checks, all reverted:
    • Renaming BaseCursor.get_default_converter reports the 12 marked overrides ("marked as an override, but no base method was found").
    • Removing @override from AthenaTypeCompiler.visit_FLOAT reports explicit-override.
    • Removing @override from Requirements.comment_reflection, or renaming it or AthenaStruct._static_cache_key, is not reported (unannotated; see the limits above).
  • Runtime: all 89 pyathena modules import together, and each of the 55 changed modules imports in a fresh interpreter (also under Python 3.11 for pyathena and the SQLAlchemy modules). override(f) is f. The marked arraysize and preparer are still properties, and arraysize keeps its setter. All 39 Requirements() properties give the same exclusion types and enabled values as on master. AioBaseCursor._poll is still a coroutine function.
  • Offline tests (pytest --noconftest with 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 the formatter and cursor fixtures from conftest.
  • sphinx-build -b html docs before and after: same warnings. The rendered API pages are the same apart from a timestamp default value.
  • Not run locally: the AWS integration suites. Nothing changes at runtime, and they run in CI when the PR is marked Ready.

🤖 Generated with Claude Code

Comment thread pyproject.toml
warn_unreachable = true
warn_unused_configs = true
# Overrides are marked with pyathena.util.override (PEP 698).
enable_error_code = ["explicit-override"]

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Self-review round 1 (implementation behavior): CLEAN

Base 39f22dc1d711bdd460adbbfd1736d0bff347ec80, head 3a134a9d7712621f09b8df98fe1ae3211b386016. All 56 changed files covered.

Covered:

  • Runtime behavior: pyathena.util.override returns the function unchanged, so the decorator order (below @property/@classmethod/@staticmethod, above @reflection.cache) does not change any descriptor. AthenaStruct._static_cache_key and _ArrayUpdate._from_objects stay plain properties, as on master. The base attributes are SQLAlchemy's _memoized_property and _non_memoized_property, which is unchanged.
  • Import cycles: each of the 55 changed pyathena modules imports on its own in a fresh interpreter. import pyathena now loads pyathena.util. That works because the package binds DataError from pyathena.error before importing pyathena.util, which imports DataError from the package.
  • SQLAlchemy test plugin (setup.cfg requirement_cls): all 39 Requirements properties give the same exclusion types and enabled values as on master.
  • Framework contracts: no dialect, reflection, or fsspec code path changes. The fsspec overrides are not touched.
  • Type checking: explicit-override reports 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 lint runs.

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>

class Requirements(SuiteRequirements):
@property
@override

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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-override false 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-version 3.12, 3.13 and 3.14. pyathena and the SQLAlchemy modules import under 3.11.
  • Documentation: no docs mention @override yet. The policy goes into docs/contributing.md in the ruff PR.
  • AWS operator: not applicable. No runtime or AWS request changes.

Comment thread pyathena/__init__.py

from pyathena.error import * # noqa: F403
from pyathena.options import ExecuteOptions as ExecuteOptions
from pyathena.util import override

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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. DataError is exported before pyathena.util imports 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.

@laughingman7743
laughingman7743 marked this pull request as ready for review October 1, 2026 17:16
@laughingman7743
laughingman7743 merged commit 9ef49d3 into master Oct 2, 2026
13 checks passed
@laughingman7743
laughingman7743 deleted the chore/882-override-all branch October 2, 2026 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant