Check docstrings in pyathena/ with ruff's pydocstyle rules - #919
laughingman7743 wants to merge 3 commits into
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.
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>
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.
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>
| - 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.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
WHAT
D) withconvention = "google":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.per-file-ignoreslists, per file, the codes that still have findings: 83 files, 508 findings (D102 367, D100 63, D107 63, D104 15). Entries are removed as the remaining PRs for Restore docstring coverage and enforce it with ruff's pydocstyle rules #882 fill the docstrings. A gap of another code, or in a file without an entry (including a new file), failsjust lint. A new gap of a listed code in a listed file is not reported until that entry is removed. For example, an undocumented method added tomodel.py, which lists D102, passes.*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, and the SQLAlchemy test-suiteRequirements.Cursor.executedocuments its seven missing arguments (work_group,s3_staging_dir,cache_size,cache_expiration_time,result_reuse_enable,result_reuse_minutes,paramstyle), in the wording the format cursors use. It is marked@override, so ruff no longer reports this (ignore-decoratorsskips every D rule, including D417).AsyncCursor.executedescribedcache_sizeas "Query result cache size in MB", andAioCursor.executeas "Query result cache size". It is the number of recent queries searched for a cached result. Both now use the wording of the other cursors.docs/contributing.mdgets a "Write docstrings" section with the rules, andAGENTS.md(CLAUDE.md) links to it.WHY
Part of #882 (step 2 of the agreed plan). Nothing checked docstrings, so coverage drifted after #601. With the rules enabled, ruff reports new gaps outside the listed codes now, and each file is checked fully once its entry is removed, package by package.
TEST
Tested commit: da04a62. It adds two documentation-only repair commits after review: a file's
Dcodes are removed while its other codes stay, and ruff checks existing private docstrings.just lintandjust docs lintwere re-run on it. The other results below are from 06ad690/824650f9, which have the same code and configuration.just format,just lint: passed (ruff with the new rules, ruff format, mypy, cfn-lint, license headers).just docs lint: 0 errors.pyathena/zz_tmp.pywithout docstrings reports D100 and D103.pyathena/util.pyreports D101 and D102.tests/pyathena/util.pyreports nothing.pyathena/model.py(which lists D102) reports nothing.ruff check pyathena --select D417,D415,D101withoutignore-decorators: no findings, so no marked override has an incompleteArgs:section either.sphinx-build -b html docsbefore and after: same warnings. The new section has thewrite-docstringsanchor thatAGENTS.mdlinks to.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