Skip to content

chore: add ruff-extra-rules linter/formatter - #410

Merged
alessio-locatelli merged 1 commit into
requests-cache:mainfrom
alessio-locatelli:add_ruff-extra-rules
Aug 26, 2026
Merged

chore: add ruff-extra-rules linter/formatter#410
alessio-locatelli merged 1 commit into
requests-cache:mainfrom
alessio-locatelli:add_ruff-extra-rules

Conversation

@alessio-locatelli

@alessio-locatelli alessio-locatelli commented Aug 22, 2026

Copy link
Copy Markdown
Collaborator

Background: During code review, I've noticed that I repeat the same feedback for both coding agents and human developers. Some of these rules are either not yet implemented in linters or still under development, so I decided to fill this gap.

I'm flexible on the outcome:

  • This is rejected with a reason
  • This is kept as an addition to ruff

If there will be a green light on this, I would like to submit an equivalent PR for https://github.com/requests-cache/requests-cache/

Comment thread pyproject.toml
[tool.ruff-extra-rules]
fix = true

[tool.ruff-extra-rules.per-file-ignores]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I ran this locally without the per-file ignores and didn't see any changes; did something previously get flagged, or is this just pre-emptive?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I submitted https://github.com/requests-cache/aiohttp-client-cache/pull/406/commits beforehand, which fixed every finding that didn't touch the public interface. This PR ended up as just the bare pre-commit hook addition on top of that.

The four remaining validate-function-name (TR4) findings are on public library functions, and they're what the per-file-ignores block exists for — the comment right above it explains why: renaming a public function isn't worth breaking the interface now, so it's deferred to the next major release. So no, it isn't pre-emptive.

Removing that block and running prek run -a on this branch reproduces exactly those four:

prek output
Extra Python rule checks (ruff-extra-rules)..............................Failed
- hook id: ruff-extra-rules
- description: Run multiple AST-based checks in a single pass for improved performance. Excludes redundant-type-conversion (TR6) -- see the ruff-extra-rules-ty hook
- exit code: 1

  aiohttp_client_cache/cache_control.py:152:1: TR4: Function 'get_expiration_datetime' should be renamed to 'calculate_expiration_datetime' (aggregates or computes a summary)
  aiohttp_client_cache/cache_control.py:168:1: TR4: Function 'get_cache_directives' should be renamed to 'extract_cache_directives' (extracts/collects data (returns list/dict))
  examples/precache.py:45:1: TR4: Function 'get_page_links' should be renamed to 'extract_page_links' (extracts/collects data (returns list/dict))
  aiohttp_client_cache/backends/base.py:274:1: TR4: Function 'get_urls' should be renamed to 'iter_urls' (generator/iterator)

That's the opposite of what you saw. You already approved and said to go ahead and merge, so I'm not blocking on this — but I'd still like to understand the discrepancy: could you share the exact edit you made to pyproject.toml and confirm you ran prek run -a (not, say, ruff-extra-rules scoped to changed files only)?

@JWCook JWCook left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Interesting! So far I've just looked over your docs and didn't dive into the code, but ruff-extra-rules looks carefully thought out. I don't think I've seen semantic analysis in a linter before (redundant-assignment/TR5 rule). TR3 looks useful; that one has bitten me before.

For rule TR7, how does it identify which line is the relevant expression that a comment belongs to, if there are multiple expressions within the bracket?

No objections here; go ahead and merge this if you'd like.

@alessio-locatelli

alessio-locatelli commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks for taking a look!

Background on TR7: While working on several large codebases, I observed that enforcing the configured line-length limit caused Black and Ruff to automatically reformat inline comments associated with specific parameters by relocating them to the closing bracket. This significantly reduced code readability and necessitated manual inspection via grep to identify and correct misplaced comments. Approximately 90% of these instances involved single-argument statements that could have been automatically resolved had a dedicated rule been available at the time.

On TR7: it's purely positional, no attempt to resolve which argument the comment describes.

  1. It only fires when the closing-bracket line contains nothing but closing brackets — is_bracket_only_line rejects any line where an expression shares the line with the bracket (so foo(a, b) # comment is untouched; only the bracket-on-its-own-line shape is in scope).
  2. When it fires, the fix always attaches the comment to whichever line is physically immediately above the bracket-only line — prev_line_idx = bracket_line_idx - 1. With multiple expressions inside the bracket, that's always the last one listed.

The preceding-placement test case shows this with two arguments — the comment lands as a preceding comment on argument_two, the line adjacent to the bracket, regardless of which argument it was originally meant to describe.

Yes, TR7 with --fix enabled may be flaky in rare cases, and I'm aware it can move the comment from one wrong position to another for multi-argument statements. --fix is opt-in on the tool itself, and even where a project turns it on by default like this one does, the developer still reviews the diff before committing it.

Glad TR3 and TR5 landed well — I'll go ahead and merge this, thanks for the review.

@alessio-locatelli
alessio-locatelli merged commit 1bfe9cb into requests-cache:main Aug 26, 2026
9 checks passed
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.

2 participants