Skip to content

Check docstrings in pyathena/ with ruff's pydocstyle rules - #919

Open
laughingman7743 wants to merge 3 commits into
masterfrom
chore/882-ruff-pydocstyle
Open

laughingman7743 wants to merge 3 commits into
masterfrom
chore/882-ruff-pydocstyle

Conversation

@laughingman7743

@laughingman7743 laughingman7743 commented Oct 2, 2026 •

Copy link
Copy Markdown
Member

WHAT

  • Enable ruff's pydocstyle rules (D) with convention = "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-ignores lists, 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), fails just 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 to model.py, which lists D102, passes.
  • Fix the findings outside that list:
    • D417: describe *args of connect() and aio_connect(). aio_connect() forwards to AioConnection.create(), which takes keyword arguments only, so the docstring says so.
    • D415: first lines of DBAPITypeObject, BaseCursor.setinputsizes and setoutputsize (which also get Args:).
    • D101: S3File, AthenaDictResultSet, and the SQLAlchemy test-suite Requirements.
  • Cursor.execute documents 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-decorators skips every D rule, including D417).
  • AsyncCursor.execute described cache_size as "Query result cache size in MB", and AioCursor.execute as "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.md gets a "Write docstrings" section with the rules, and AGENTS.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 D codes are removed while its other codes stay, and ruff checks existing private docstrings. just lint and just docs lint were 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.
  • Rule scope, checked by temporary edits that were reverted:
    • A new pyathena/zz_tmp.py without docstrings reports D100 and D103.
    • An undocumented class with a method appended to pyathena/util.py reports D101 and D102.
    • The same class appended to tests/pyathena/util.py reports nothing.
    • The same class, with a class docstring, appended to pyathena/model.py (which lists D102) reports nothing.
  • ruff check pyathena --select D417,D415,D101 without ignore-decorators: no findings, so no marked override has an incomplete Args: section either.
  • sphinx-build -b html docs before and after: same warnings. The new section has the write-docstrings anchor that AGENTS.md links to.
  • Rendered API reference: PandasCursor.get_default_converter has no docstring of its own and shows BaseCursor.get_default_converter's, as the contributing guide states.
  • aio_connect("s3://x/") raises TypeError: AioConnection.create() takes 1 positional argument but 2 were given, as the new *args description states.
  • Not run locally: the AWS integration suites. Only docstrings and configuration change, and they run in CI when the PR is marked Ready.

🤖 Generated with Claude Code

Comment thread pyproject.toml
[tool.ruff.lint.pydocstyle]
convention = "google"
# Overrides marked with @override inherit the base method's documentation.
ignore-decorators = ["pyathena.util.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 9ef49d311a7124ef90a58703d02b14e612a17bdc, head 06ad69063963497fd2d6423ad9cfca296b18c067. All 11 changed files covered.

Covered:

  • Rule scope: !pyathena/** disables D outside the package, so tests/, scripts/ and docs/conf.py are not checked. Inside the package, a new file and an undocumented class are reported (mutations reverted). ignore-decorators exempts @override methods. D105 is 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) follows Connection.__init__'s parameter order (s3_staging_dir, region_name, ...) in both overloads and the implementation.
    • aio_connect(*args) raises TypeError because AioConnection.create(cls, **kwargs) takes keyword arguments only.
    • S3FileSystem._open returns S3File.
    • AthenaDictResultSet._get_rows builds rows with dict_type from the column names.
    • DBAPITypeObject.__eq__ uses other in self.
    • cache_size is the number of recent executions searched (common.py _find_previous_query_id), not a size in MB.
    • setinputsizes/setoutputsize are empty in BaseCursor. AsyncAdapt_pyathena_cursor.setinputsizes in 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>
@laughingman7743
laughingman7743 force-pushed the chore/882-ruff-pydocstyle branch from 06ad690 to 824650f Compare October 2, 2026 00:30
Comment thread pyproject.toml Outdated
"D102",
"D107",
]
# Missing docstrings (#882). Remove an entry once its file is documented.

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 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.toml and ruff output.
  • ruff --select D417,D415,D101 without ignore-decorators passes, so no marked override has an incomplete Args: section. D417 only checks docstrings that have an Args: 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_docstrings is not set in docs/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.
  • Documentation reader: no other docs state docstring rules. CONTRIBUTING.md links to docs/contributing.md. The AGENTS.md anchor 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>
Comment thread docs/contributing.md Outdated
- 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.

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): 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:

  1. docs/contributing.md:91 — Removing an entire entry can remove required naming exemptions.
    The entries for pyathena/sqlalchemy/compiler.py and pyathena/aio/sqlalchemy/base.py contain N802 and N801, respectively, alongside the new D codes. Following this instruction after completing their docstrings would expose existing names such as visit_TINYINT and AsyncAdapt_pyathena_cursor to lint failures. Instruct contributors to remove resolved D codes while preserving unrelated exemptions.

  2. 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 under pyathena/ 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 calling start_query_execution, but execute() 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 imported override. 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:

  1. Confirmed. compiler.py and aio/sqlalchemy/base.py keep N802/N801 in the same entries. Fixed in 0e3b2d8: the guide and the pyproject.toml comment now say to remove a file's D codes and keep its other codes.
  2. Confirmed. """Return the result""" on a private helper appended to pyathena/util.py reports D415 (reverted). Fixed in 0e3b2d8: ruff does not require private docstrings, but checks the format of those that exist.
  3. Confirmed and pre-existing. BaseCursor._execute returns a cached query ID from _find_previous_query_id without calling start_query_execution, but Cursor.execute calls the callback anyway. The same description is in the execute docstrings 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.

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.

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 D codes 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 whose Args: 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 lint and just docs lint pass.
  • 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 D codes, 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>
@laughingman7743
laughingman7743 marked this pull request as ready for review October 2, 2026 00:42

This branch has not been deployed

No deployments
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