Document every public API in pyathena/ and check docstrings with ruff - #919
Conversation
| [tool.ruff.lint.pydocstyle] | ||
| convention = "google" | ||
| # Overrides marked with @override inherit the base method's documentation. | ||
| ignore-decorators = ["pyathena.util.override"] |
There was a problem hiding this comment.
Self-review round 1 (implementation behavior): CLEAN
Base 9ef49d311a7124ef90a58703d02b14e612a17bdc, head 06ad69063963497fd2d6423ad9cfca296b18c067. All 11 changed files covered.
Covered:
- Rule scope:
!pyathena/**disables D outside the package, sotests/,scripts/anddocs/conf.pyare not checked. Inside the package, a new file and an undocumented class are reported (mutations reverted).ignore-decoratorsexempts@overridemethods.D105is ignored. The per-file entries match the measured findings (83 files, 508 findings) and do not hide the D101/D415/D417 codes. - Docstring facts checked against the code:
connect(*args)followsConnection.__init__'s parameter order (s3_staging_dir,region_name, ...) in both overloads and the implementation.aio_connect(*args)raisesTypeErrorbecauseAioConnection.create(cls, **kwargs)takes keyword arguments only.S3FileSystem._openreturnsS3File.AthenaDictResultSet._get_rowsbuilds rows withdict_typefrom the column names.DBAPITypeObject.__eq__usesother in self.cache_sizeis the number of recent executions searched (common.py_find_previous_query_id), not a size in MB.setinputsizes/setoutputsizeare empty inBaseCursor.AsyncAdapt_pyathena_cursor.setinputsizesin the async dialect is a separate adapter method and is unaffected.
- Runtime: only docstrings and configuration change. No code path changes.
- Tests: none needed. The guard is
just lint, and the rule scope was checked by mutation.
Findings: none.
06ad690 to
824650f
Compare
| "D102", | ||
| "D107", | ||
| ] | ||
| # Missing docstrings (#882). Remove an entry once its file is documented. |
There was a problem hiding this comment.
Self-review round 2 (claims, callers, evidence): FINDINGS (repaired)
Base 9ef49d311a7124ef90a58703d02b14e612a17bdc, head 06ad69063963497fd2d6423ad9cfca296b18c067 → 824650f9b0582f93319b8bbbbc535dff0953ca38 (same tree, commit message only).
Finding: the PR body said that any other gap fails just lint, and the commit message said that new code cannot add more gaps. Both overstate the check. These ignores apply per file and per code, so a new undocumented method in a file that lists D102 is not reported. Verified: appending a documented class with an undocumented method to pyathena/model.py (which lists D102) passes ruff. The edit was reverted.
Repair: the commit message and PR body now say that a gap of an unlisted code, or in a file without an entry, fails; a listed code in a listed file is not reported until that entry is removed. The model.py case is added to TEST.
Other claims checked:
- 83 files and 508 findings by code, recounted from
pyproject.tomland ruff output. ruff --select D417,D415,D101withoutignore-decoratorspasses, so no marked override has an incompleteArgs:section. D417 only checks docstrings that have anArgs:section, and the wording claims only that.- Contributing guide:
- "the API reference shows the base method's docstring" was checked in the rendered HTML (
PandasCursor.get_default_converter). Autodoc inherits docstrings by default (autodoc_inherit_docstringsis not set indocs/conf.py). - "mypy reports a missing decorator, except on an unannotated property" matches the minimal reproduction from Mark every checkable override with @override #918: an unannotated method is reported, an unannotated property is not. Untyped fsspec bases are covered by the next bullet.
- "the API reference shows the base method's docstring" was checked in the rendered HTML (
- Documentation reader: no other docs state docstring rules.
CONTRIBUTING.mdlinks todocs/contributing.md. TheAGENTS.mdanchor exists in the built HTML and in GitHub's heading slug. - Existing callers and AWS operator: not applicable. Only docstrings and lint configuration change.
| - New or changed private functions and methods use the same style, although ruff does not check them. | ||
|
|
||
| `per-file-ignores` in `pyproject.toml` lists the files that still lack docstrings. | ||
| Remove a file's entry when its docstrings are complete. |
There was a problem hiding this comment.
Independent review (relayed): FINDINGS
Reviewer: Codex CLI 0.159.3, model gpt-6-astra, reasoning effort high, sandbox read-only, session 01a0fa05-568a-7133-9633-2f680781c761. It did not author the change.
Scope: base 9ef49d311a7124ef90a58703d02b14e612a17bdc, head 824650f9b0582f93319b8bbbbc535dff0953ca38. The reviewer read a detached clean checkout and the literal diff. Its prompt left out the PR number, description, commit messages and earlier findings. Constraints: no edits, builds, tests, linters, network, GitHub or memory. This is a static review. The snapshot and PR worktree were unchanged afterward.
Reviewer's result (verbatim; local file links point to the review snapshot):
Surfaces covered:
- Ruff selection, Google convention, D105, negated glob, combined per-file ignores, dotted decorator matching, and the pinned version.
- Every changed docstring and its implementation, including connection forwarding, execution options, cache lookup, DB API no-ops, S3 files, dictionary results, and SQLAlchemy requirements.
- Contributor instructions,
override, mypy configuration,AGENTS.md/CLAUDE.md, and Sphinx/napoleon settings.Verdict: FINDINGS
Introduced issues:
docs/contributing.md:91 — Removing an entire entry can remove required naming exemptions.
The entries forpyathena/sqlalchemy/compiler.pyandpyathena/aio/sqlalchemy/base.pycontainN802andN801, respectively, alongside the new D codes. Following this instruction after completing their docstrings would expose existing names such asvisit_TINYINTandAsyncAdapt_pyathena_cursorto lint failures. Instruct contributors to remove resolved D codes while preserving unrelated exemptions.docs/contributing.md:88 — Ruff still checks existing private docstrings.
Private functions are exempt from missing-docstring requirements, but their supplied docstrings remain subject to applicable formatting rules. For example, an undecorated private helper underpyathena/with"""Return the result"""can trigger Google-convention D415 for missing terminal punctuation. Replace “ruff does not check them” with the narrower missing-docstring exemption.Pre-existing issue within a touched docstring:
- pyathena/cursor.py:117 — The callback also runs on client-side cache hits.
_execute()can return an existing query ID without callingstart_query_execution, butexecute()invokes the callback unconditionally. With a matching cached query, monitoring code relying on this description could record a new execution that never occurred. Document invocation after obtaining either a new or cached query ID.Based on my knowledge of Ruff’s documented behavior, the configuration otherwise matches the intent: the negated pattern excludes D rules outside
pyathena/, matching ignores combine, and the qualified decorator name matches the importedoverride. The newly written API descriptions are consistent with their implementations; I found no introduced napoleon formatting defect or runtime behavior change.This was static inspection only. Ruff’s exact remaining-findings inventory and rendered documentation were not executed or verified.
Author verification:
- Confirmed.
compiler.pyandaio/sqlalchemy/base.pykeep N802/N801 in the same entries. Fixed in 0e3b2d8: the guide and thepyproject.tomlcomment now say to remove a file's D codes and keep its other codes. - Confirmed.
"""Return the result"""on a private helper appended topyathena/util.pyreports D415 (reverted). Fixed in 0e3b2d8: ruff does not require private docstrings, but checks the format of those that exist. - Confirmed and pre-existing.
BaseCursor._executereturns a cached query ID from_find_previous_query_idwithout callingstart_query_execution, butCursor.executecalls the callback anyway. The same description is in theexecutedocstrings of 13 cursor modules, none of whose lines this PR changes. Deferred to the Restore docstring coverage and enforce it with ruff's pydocstyle rules #882 PR that fills those cursor modules, so that all 13 change together.
There was a problem hiding this comment.
Repair record
Commits: 0e3b2d861b1a983fb9468dd167cf20ba7830f066 and da04a626360852f13a7b825e13e11eca02e82312, on top of the reviewed head 824650f9. They change only docs/contributing.md and a pyproject.toml comment.
- Findings 1 and 2: the guide says to remove a file's
Dcodes and keep its other codes, and says ruff does not require private docstrings but checks those that exist. The wording is "checks the docstrings they have", not only their format, because D417 also applies: a private helper whoseArgs:section misses an argument reports D417 (temporary edit, reverted). - Finding 3: deferred as stated above. It is a pre-existing problem in 13 cursor modules.
Self-review of the repair:
- Behavior: only documentation and a TOML comment change.
just lintandjust docs lintpass. - Claims: the N801/N802 entries were checked in
pyproject.toml, and the private D415/D417 behavior on ruff 0.14.14 with temporary edits.
Independent follow-up, relayed: Codex CLI 0.159.3, model gpt-6-astra, effort high, sandbox read-only, session 01a0fa09-4fda-7711-a17b-22a5c5c2eeaf. Range 824650f9b0582f93319b8bbbbc535dff0953ca38..da04a626360852f13a7b825e13e11eca02e82312, static review:
Verdict: CLEAN. Both earlier findings are resolved: Guidance removes only
Dcodes, preserving the shared N801/N802 naming exemptions. Private-function guidance correctly distinguishes docstring presence requirements from checks on existing docstrings, including D415/D417. The wording is consistent with the surrounding section and configuration. No new actionable inaccuracies found.
da04a62 to
b59c120
Compare
| def info(self, path: str, **kwargs) -> S3Object: | ||
| """Return information about an S3 path. | ||
|
|
||
| Uses the directory cache first: a cached entry for the path is |
There was a problem hiding this comment.
Self-review round 1 (implementation behavior), full pass after the scope change: FINDINGS (repaired)
Base 2fcbcdbe (master after #853), head 358d15542471bdbc3cadb1678ba7d5c04c1397ae. Scope: all 87 changed files, including the 508 new docstrings written by six parallel helper agents (one per package). This is self-review by the authoring model.
Covered: every added docstring, read against the code it describes. I checked the non-trivial claims in the source:
- model
DataErrorkeys and state constants; table-metadata compression order,row_format/file_format, and the case-insensitive checks. - result-set
_pre_fetch,description, type-hint lookup by index and then by lowercased name, andas_pandas. S3FileSystem.info/touch/rm/cat_file/invalidate_cache,S3File.__init__,S3Object.__init__/to_api_repr, and theAioS3FileSystemforwarding.visit_struct/_enable_hive_column_ddl,visit_getitem_binaryslice/step/index, andAthenaDialect.__init__option precedence.- the
Converter/RetryConfigjitter,AsyncPandasCursor.arraysize, the Spark default engine configuration, andcancel.
The no-code-change check (AST without docstrings, against master) still passes for all 83 modules.
Findings, all repaired in 358d155:
S3FileSystem.infosaid a cache miss goes to HeadObject. A cached listing of the path itself makes it a directory, and a cached parent listing without it raisesFileNotFoundError, both without a request.S3File.__init__said a large existing object is copied for append. It is copied only once a multipart upload starts.- Seven new
kill_on_interruptdescriptions predated Re-raise and stop interrupted queries in the SQL cursors, sharing the Spark handling #853 and said "while polling". They are now aligned by cursor family: SQL cursors cancel on an interrupt while starting or waiting; asyncio cursors cancel on task cancellation while starting or waiting;AsyncCursorvariants cover only the start.
Limitations: docstrings that describe AWS API fields (statistics meanings, engine versions) follow the AWS API reference, not code. Pre-existing docstring errors and suspected bugs reported by the helpers are out of scope (see PR body).
| PyAthena provides a callback mechanism that allows you to get immediate access to the query ID | ||
| as soon as the `start_query_execution` API call is made, before waiting for query completion. | ||
| This is useful for monitoring, logging, or cancelling long-running queries from another thread. | ||
| When `cache_size` finds a reusable query, no new query starts, and the callback receives the reused query's ID. |
There was a problem hiding this comment.
Self-review round 2 (claims, callers, evidence), full pass: CLEAN after PR-body updates
Base 2fcbcdbe, head 358d15542471bdbc3cadb1678ba7d5c04c1397ae.
Claims checked:
- Counts: an AST comparison with master finds 511 definitions that gained a docstring (78 modules/packages, 63
__init__, 279 property getters, 91 classes/functions/methods). That is the 508 D findings plus the three D101 classes. The PR body now states it that way (the helpers' self-reported totals were higher). - No code change: the AST without docstrings equals master's for all 83 modules. The only removed statement is the PIE790
pass. - Eleven previously empty
__init__.pyfiles now carry the 2026 header (checked withgit show origin/master:<f> | wc -c).scripts/check_license_headers.pypasses. on_start_query_execution: all ten cursors call_call_on_start_query_executionright after_execute(), and_execute()returns a cached ID from_find_previous_query_idwithoutStartQueryExecution. The new wording, and the sentence added todocs/usage.md(anchored here), match that. The same sentence is not added todocs/sqlalchemy.md, because the SQLAlchemy dialects never passcache_size.kill_on_interrupt: checked againstBaseCursor._poll/_start_executionandAioBaseCursorafter Re-raise and stop interrupted queries in the SQL cursors, sharing the Spark handling #853, and againstdocs/usage.md"Query cancellation on interrupt" anddocs/aio.md"Task cancellation". The PR body bullet was stale (it named only two docstrings) and is updated.- Rendered docs on this head: 145 Sphinx warnings, none new against master's 151.
ruff --select D417,D415,D101withoutignore-decoratorspasses. - Commit messages: the claims in
Check every docstring...,Say when on_start_query_execution...andAlign the new docstrings...match the diff. - Existing callers and AWS operator: only docstrings, Markdown and lint configuration change. Runtime behavior is unchanged (AST check).
Deferred, as agreed with the maintainer: pre-existing docstring errors go to a separate PR, and suspected code bugs will be verified and then filed as issues.
Enable ruff's D rules with the Google convention for pyathena/. Methods decorated with pyathena.util.override are exempt, and magic methods need no docstring. Files outside the package are not checked. per-file-ignores lists, per file, the codes that still have findings. A gap of another code, or in a file without an entry, fails the check; a new gap of a listed code in a listed file is not reported until that file's entry is removed. Fix the findings outside that list: complete Cursor.execute's Args, describe *args of connect() and aio_connect(), end the first lines of DBAPITypeObject, setinputsizes and setoutputsize with a period, and add docstrings to S3File, AthenaDictResultSet and the SQLAlchemy test-suite Requirements. Correct cache_size in the AsyncCursor and AioCursor execute docstrings, which described it as a cache size rather than the number of queries to check. Document the docstring rules in the contributing guide. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The compiler and asyncio dialect entries also hold naming exemptions, so completing a file removes its D codes, not the whole entry. ruff also checks the format of private docstrings that exist, so the guide says it only does not require them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
All public modules, packages, classes, functions, methods, property getters and __init__ methods in pyathena/ now have docstrings, so the per-file ignores for the D rules are no longer needed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The cursors call on_start_query_execution once _execute() returns a query ID, which is either a new query from StartQueryExecution or a reusable query found through cache_size. The docstrings and the usage guide said it runs right after StartQueryExecution or "when the query starts", which does not hold for a cache hit. Since #853, kill_on_interrupt also cancels a query whose start is interrupted. AsyncCursor waits on worker threads, which do not receive KeyboardInterrupt, so it only covers the start there. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The new kill_on_interrupt descriptions predate #853: the SQL cursors also cancel a query whose start is interrupted, the asyncio cursors react to task cancellation while starting or waiting, and the AsyncCursor variants only cover the start. S3FileSystem.info() also treats a cached listing of the path as a directory and a cached parent listing without it as missing, and S3File copies a large existing object for append only once a multipart upload starts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
358d155 to
35fad82
Compare
Pandas chunksize only applies to CSV results (UNLOAD reads the whole Parquet output), Polars reads lazily only from result files in S3, and description is None only for INSERT, UPDATE, DELETE and MERGE, not for every DML statement type. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| ) -> list[tuple[str, str, None, None, int, int, str]] | None: | ||
| """The DB API 2.0 column descriptions. | ||
|
|
||
| None without result metadata, or for ``INSERT``, ``UPDATE``, ``DELETE``, and ``MERGE``. |
There was a problem hiding this comment.
Independent review (relayed), full scope: FINDINGS (repaired)
Reviewer: Codex CLI 0.159.3, model gpt-6-astra, reasoning effort high, sandbox read-only, session 01a0ffa8-c667-7720-8af3-70a955377565. It did not author the change. Scope: base 2fcbcdbe7ae01b31a4a3bb613e6bccdfcd382014, head 358d15542471bdbc3cadb1678ba7d5c04c1397ae, full diff (about 5200 lines). The reviewer read a detached clean checkout with the dependency sources in .venv available. Its prompt left out the PR number, description, commit messages and earlier findings. Constraints: no edits, builds, tests, network, GitHub or memory. This is a static review. The snapshot was unchanged afterward.
Since the review, the branch was rebased onto master 775874c (#920, docs only). git range-diff shows all 12 patches unchanged, and #920's cancellation wording does not conflict with the new docstrings.
Reviewer's result (verbatim; file links point to the review snapshot):
Surfaces covered: Read the full diff. Closely traced cursor constructors, callback/cache/interrupt behavior, result-set loading, S3 filesystem operations, SQLAlchemy adapters/types/compiler methods, and retry helpers. Sampled model and S3 response properties. Inspected Ruff configuration and Napoleon conventions statically; no checks were run.
Verdict: FINDINGS
[P2] pyathena/aio/polars/cursor.py:81 — Chunking is not always lazy.
The new description promises lazy reads wheneverchunksizeis set. Withoutoutput_location,AthenaPolarsResultSetinstead calls_as_polars_from_api()during construction (pyathena/polars/result_set.py:262), which fetches every row and builds one DataFrame (:527). A user selectingchunksize=1000for a large managed-results query would still load the entire result duringexecute(). Qualify the promise as applying to S3-backed results.[P2] pyathena/aio/pandas/cursor.py:82 — Pandas chunk-size documentation omits the UNLOAD limitation.
Withunload=True,_as_pandas()calls_read_parquet()(pyathena/pandas/result_set.py:785), which callspd.read_parquet()without applying_chunksize(:751). The iterator then yields that entire DataFrame once (:86). Thusunload=True, chunksize=1000does not produce 1,000-row chunks and can require memory for the full result. Limit this description—and the newly added equivalent atpyathena/pandas/async_cursor.py:102—to CSV reading.[P3] pyathena/result_set.py:374 — “None … for DML” is too broad.
The implementation suppresses descriptions only forINSERT,UPDATE,DELETE, andMERGEsubstatement types (:60,:375). Athena also classifiesSELECTas DML, as the existing SELECT test explicitly asserts (tests/pyathena/test_cursor.py:121). A reader could incorrectly conclude that SELECT results lack column descriptions. Name the four modifying statement types instead.
Author verification, all confirmed and fixed in 6be315e:
AthenaPolarsResultSetcalls_as_polars_from_api()when there is nooutput_location(polars/result_set.py:260-263). Thechunksizedescription now says "result files in S3 are read lazily"._read_parquet()callspd.read_parquet()withoutchunksize. TheAioPandasCursorandAsyncPandasCursorchunksizedescriptions now say "when reading CSV results"._DML_SUBSTATEMENT_TYPESis{INSERT, UPDATE, DELETE, MERGE}. Thedescriptiondocstring now names those four.
After the repair:just lintpasses, and the AST check without docstrings still equals master for all 83 modules.
There was a problem hiding this comment.
Repair record
Repair commit: 6be315ec4c87758f993778b0b2167696e848390a. It changes only the four docstrings named above.
Self-review of the repair:
- Behavior: docstrings only.
just lintpasses, and the AST check still matches master. - Claims: checked against
polars/result_set.py:260-263(API fallback),pandas/result_set.py_read_parquet()(nochunksize), and_DML_SUBSTATEMENT_TYPES.
Independent follow-up, relayed: Codex CLI 0.159.3, model gpt-6-astra, effort high, sandbox read-only, session 01a0ffae-9fe5-7283-bae5-76c90fdc2fb4. Range 35fad823de412d8d5bb5d06bb067cf2535bd18e5..6be315ec4c87758f993778b0b2167696e848390a, static review:
CLEAN — all three earlier findings are resolved. No new inaccuracy or formatting problem found. Static inspection only; no execution-based validation.
WHAT
D) withconvention = "google"forpyathena/:ignore-decorators = ["pyathena.util.override"]: methods marked as overrides (Mark every checkable override with @override #918) are exempt.D105(magic methods) is ignored."!pyathena/**" = ["D"]: files outside the package (tests/,scripts/,docs/conf.py) are not checked.__init__inpyathena/has a docstring.__init__docstrings describe every argument, including where*args/**kwargsgo.filesystem/s3.pyands3_async.pyare documented because they cannot carry@override(fsspec has no type information).passremoved fromAsyncAdapt_pyathena_connection.rollback, which ruff's PIE790 flags once the method has a docstring.__init__.pyfiles were empty and now contain a docstring, so they get the 2026 license header. Their earliest content is from 2026, so this followsdocs/contributing.md.*argsofconnect()andaio_connect().aio_connect()forwards toAioConnection.create(), which takes keyword arguments only, so the docstring says so.DBAPITypeObject,BaseCursor.setinputsizesandsetoutputsize(which also getArgs:).S3File,AthenaDictResultSet, the SQLAlchemy test-suiteRequirements.Cursor.executedocuments its seven missing arguments, in the wording the format cursors use. It is marked@override, andignore-decoratorsskips D417 too, so ruff did not report them.cache_sizeinAsyncCursor.execute("cache size in MB") andAioCursor.execute("cache size") is the number of recent queries searched for a reusable result.on_start_query_executionwas described as running right afterStartQueryExecution, or "when the query starts". The cursors call it once_execute()returns a query ID, and withcache_sizethat can be a reused query that starts nothing. This is fixed in the tenexecute()docstrings,ExecuteOptions,Connection,BaseCursor, and the "Query execution callback" section ofdocs/usage.md.kill_on_interruptin the new__init__docstrings follows Re-raise and stop interrupted queries in the SQL cursors, sharing the Spark handling #853, by cursor family. The SQL cursors cancel the query on aKeyboardInterruptwhile starting or waiting. The asyncio cursors cancel it when the task is cancelled while starting or waiting. TheAsyncCursorvariants cover only the start, because they wait on worker threads, which do not receiveKeyboardInterrupt.docs/contributing.mdgets a "Write docstrings" section with the rules, andAGENTS.md(CLAUDE.md) links to it.Pre-existing docstring errors found while writing these (wrong examples, attributes that do not exist, and similar) are not fixed here and will follow in a separate PR. Code bugs found on the way were verified and filed as #921, #922, #923, #924, #925 and #926.
WHY
Closes #882. Nothing checked docstrings, so coverage drifted after #601. Filling every gap now and enabling the rules without exceptions means ruff reports any new gap in
pyathena/.TEST
Tested commit: 6be315e (rebased on master 775874c)
just format,just lint: passed (ruff with the D rules and no per-file D exceptions, ruff format, mypy, cfn-lint, license headers).just docs lint: 0 errors.pyathena/, the AST with docstrings andpassstatements removed equals master's.sphinx-build -b html docson master and on this head: no new warnings. The count drops from 151 to 145, because six warnings came from fsspec and SQLAlchemy docstrings that the API reference inherited and that the new docstrings now replace (S3File.__init__,S3FileSystem.cat_file,AioS3FileSystem.__init__,AthenaDDLCompiler.__init__).pyathena/zz_tmp.pywithout docstrings reports D100 and D103.pyathena/util.pyreports D101 and D102.tests/pyathena/util.pyreports nothing.ruff check pyathena --select D417,D415,D101withoutignore-decorators: no findings, so no marked override has an incompleteArgs:section either.PandasCursor.get_default_converterhas no docstring of its own and showsBaseCursor.get_default_converter's, as the contributing guide states.aio_connect("s3://x/")raisesTypeError: AioConnection.create() takes 1 positional argument but 2 were given, as the new*argsdescription states.🤖 Generated with Claude Code