[`flake8-commas`] Add support for trailing comma checks in type parameter lists (`COM812`, `COM819`) by danparizher · Pull Request #19390 · astral-sh/ruff

danparizher · 2025-07-16T22:42:53Z

Summary

I'm not too sure if the solution is as simple as the way I implemented it, but I'm curious to see if we are covering all cases correctly here.

github-actions · 2025-07-16T23:04:22Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

MichaReiser · 2025-07-18T13:39:33Z

We probably have to gate this behind preview. Did you have a chance to look through the ecosystem results. Do the changes look correct to you?

danparizher · 2025-07-18T17:29:52Z

Yes, the ecosystem changes look to be correct, from what I can tell. I also gave a shot at implementing the preview, but I'm not sure if I did it correctly. I would appreciate a review!

ntBre

Thanks! I haven't gone through all of the snapshots yet, but I had a couple of comments on the code in an initial pass.

This is kind of a note to myself for later, but we'll want to double check that the ecosystem check doesn't change on stable for a preview change. It looks like it didn't rerun after you added the preview gate, but hopefully it will run again after any additional commits.

crates/ruff_linter/src/checkers/tokens.rs

crates/ruff_linter/src/rules/flake8_commas/rules/trailing_commas.rs

danparizher · 2025-07-24T20:53:21Z

It looks like the ecosystem check didn't rerun. Maybe we can manually rerun it?

MichaReiser · 2025-07-25T06:50:43Z

The ecosystem results were updated 10h ago. That would suggest that they did run?

ntBre · 2025-07-25T13:18:38Z

Maybe there was a queue of ecosystem checks yesterday afternoon. It hadn't updated when I checked after the last commit here yesterday either. I see it now though!

My suggestion might have been slightly misleading about your old preview check. We'll need both the new match on two tokens and the preview check. Maybe something like this:

TokenType::OpeningSquareBracket if is_your_preview_enabled(settings) => match (prev.ty, prev_prev.ty) { // new code
    ...
}
TokenType::OpeningSquareBracket => match prev.ty { // the old code

Then hopefully all the ecosystem changes will show up in the preview section as expected.

crates/ruff_linter/src/rules/flake8_commas/rules/trailing_commas.rs

…as.rs Co-authored-by: Brent Westbrook <[email protected]>

danparizher · 2025-07-25T21:51:49Z

Appreciate the suggestions, I think I was confusing myself with this part 😆

ntBre

Thanks. I think you may have missed a couple of the key parts of my patch suggestion. There were still a lot of unexpected differences between the preview and non-preview snapshots, which I saw locally by diffing them. Hopefully we'll get #19351 at some point and this will be easier to see directly in PRs.

I made a couple of tweaks and then confirmed that

diff crates/ruff_linter/src/rules/flake8_commas/snapshots/ruff_linter__rules__flake8_commas__tests__COM81_syntax_error.py.snap crates/ruff_linter/src/rules/flake8_commas/snapshots/ruff_linter__rules__flake8_commas__tests__preview__COM81_syntax_error.py.snap

was totally empty and that

diff crates/ruff_linter/src/rules/flake8_commas/snapshots/ruff_linter__rules__flake8_commas__tests__COM81.py.snap crates/ruff_linter/src/rules/flake8_commas/snapshots/ruff_linter__rules__flake8_commas__tests__preview__COM81.py.snap

only showed modifications at the end of the file, where the new cases were added.

I think that will also cut down on the ecosystem changes. I'll double check that and then merge this.

crates/ruff_linter/src/checkers/tokens.rs

ntBre · 2025-07-28T14:52:38Z

That actually entirely cleared the ecosystem changes 😅 I hadn't noticed before that they were all related to the slice change, not actual type parameters, but that's the case for all the ones I clicked through just now.

Summary -- I noticed while reviewing #19390 that in `check_tokens` we were still passing around an extra `LinterSettings`, despite all of the same functions also receiving a `LintContext` with its own settings. This PR adds the `LintContext::settings` method and calls that instead of using the separate `LinterSettings`. Test Plan -- Existing tests

) Summary -- I noticed while reviewing #19390 that in `check_tokens` we were still passing around an extra `LinterSettings`, despite all of the same functions also receiving a `LintContext` with its own settings. This PR adds the `LintContext::settings` method and calls that instead of using the separate `LinterSettings`. Test Plan -- Existing tests

… parameter lists (`COM812`, `COM819`) (#20275) Introduced in #19390. Removed gating, updated tests. No documentation to update.

danparizher added 2 commits July 16, 2025 18:38

fix-18844

88353d9

remove unused tests

78689e3

MichaReiser added the rule Implementing or modifying a lint rule label Jul 18, 2025

apply preview

1fc3e29

MichaReiser requested a review from ntBre July 23, 2025 06:50

ntBre reviewed Jul 24, 2025

View reviewed changes

crates/ruff_linter/src/checkers/tokens.rs Show resolved Hide resolved

crates/ruff_linter/src/rules/flake8_commas/rules/trailing_commas.rs Outdated Show resolved Hide resolved

crates/ruff_linter/src/rules/flake8_commas/rules/trailing_commas.rs Outdated Show resolved Hide resolved

apply feedback

63bfeed

danparizher requested a review from ntBre July 24, 2025 20:53

fix preview(?)

76a4336

ntBre reviewed Jul 25, 2025

View reviewed changes

crates/ruff_linter/src/rules/flake8_commas/rules/trailing_commas.rs Outdated Show resolved Hide resolved

crates/ruff_linter/src/rules/flake8_commas/rules/trailing_commas.rs Outdated Show resolved Hide resolved

danparizher and others added 3 commits July 25, 2025 17:36

Update crates/ruff_linter/src/rules/flake8_commas/rules/trailing_comm…

37ce181

…as.rs Co-authored-by: Brent Westbrook <[email protected]>

Update crates/ruff_linter/src/rules/flake8_commas/rules/trailing_comm…

0ffca19

…as.rs Co-authored-by: Brent Westbrook <[email protected]>

cargo clippy, update snapshots

9136ddd

danparizher requested a review from ntBre July 25, 2025 22:34

fix unrelated preview changes by matching original code

ec998f7

ntBre reviewed Jul 28, 2025

View reviewed changes

crates/ruff_linter/src/checkers/tokens.rs Show resolved Hide resolved

ntBre changed the title ~~[flake8_commas] Add support for trailing comma checks in type parameter lists for COM812 and COM819~~ [flake8-commas] Add support for trailing comma checks in type parameter lists (COM812,COM819) Jul 28, 2025

ntBre added the preview Related to preview mode features label Jul 28, 2025

ntBre merged commit e63dfa3 into astral-sh:main Jul 28, 2025
36 checks passed

danparizher deleted the fix-18844 branch July 28, 2025 15:20

ntBre changed the title ~~[flake8-commas] Add support for trailing comma checks in type parameter lists (COM812,COM819)~~ [flake8-commas] Add support for trailing comma checks in type parameter lists (COM812, COM819) Jul 28, 2025

ntBre mentioned this pull request Jul 29, 2025

Add LinterContext::settings to avoid passing separate settings #19608

Merged

BrewTestBot mentioned this pull request Jul 29, 2025

ruff 0.12.7 Homebrew/homebrew-core#231689

Merged

dylwil3 mentioned this pull request Sep 5, 2025

[flake8-commas] Stabilize support for trailing comma checks in type parameter lists (COM812, COM819) #20275

Merged

dylwil3 added a commit that referenced this pull request Sep 8, 2025

[flake8-commas] Stabilize support for trailing comma checks in type…

c694fb4

… parameter lists (`COM812`, `COM819`) (#20275) Introduced in #19390. Removed gating, updated tests. No documentation to update.

ntBre pushed a commit that referenced this pull request Sep 8, 2025

[flake8-commas] Stabilize support for trailing comma checks in type…

b6cd1a6

… parameter lists (`COM812`, `COM819`) (#20275) Introduced in #19390. Removed gating, updated tests. No documentation to update.

ntBre pushed a commit that referenced this pull request Sep 10, 2025

[flake8-commas] Stabilize support for trailing comma checks in type…

706d51e

… parameter lists (`COM812`, `COM819`) (#20275) Introduced in #19390. Removed gating, updated tests. No documentation to update.

ntBre pushed a commit that referenced this pull request Sep 10, 2025

[flake8-commas] Stabilize support for trailing comma checks in type…

df8be78

… parameter lists (`COM812`, `COM819`) (#20275) Introduced in #19390. Removed gating, updated tests. No documentation to update.

ntBre pushed a commit that referenced this pull request Sep 10, 2025

[flake8-commas] Stabilize support for trailing comma checks in type…

ac7f882

… parameter lists (`COM812`, `COM819`) (#20275) Introduced in #19390. Removed gating, updated tests. No documentation to update.

Comments

Conversation

danparizher commented Jul 16, 2025

Summary

Uh oh!

github-actions bot commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Uh oh!

MichaReiser commented Jul 18, 2025

Uh oh!

danparizher commented Jul 18, 2025

Uh oh!

ntBre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danparizher commented Jul 24, 2025

Uh oh!

MichaReiser commented Jul 25, 2025

Uh oh!

ntBre commented Jul 25, 2025

Uh oh!

Uh oh!

Uh oh!

danparizher commented Jul 25, 2025

Uh oh!

ntBre left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ntBre commented Jul 28, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Jul 16, 2025 •

edited

Loading

`ruff-ecosystem` results

ntBre left a comment •

edited

Loading