refactor: improve group matching computation#733
Draft
hugop95 wants to merge 21 commits into
Draft
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR refactors the group computation system across ~40 rule files and utilities. It removes module-level caching mechanisms ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #733 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 224 224
Lines 4400 4416 +16
Branches 1375 1374 -1
=========================================
+ Hits 4400 4416 +16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
rules/sort-imports/types.ts (1)
151-162: Add precedence regression tests for this reordered selector/modifier registry.Because GroupMatcher compares by selector/modifier index, this reorder changes matching priority. Please lock expected precedence with focused rule tests for overlapping cases (e.g., imports that can satisfy both type/path/style-related groups).
Also applies to: 174-180
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rules/sort-imports/types.ts` around lines 151 - 162, The reordered selector/modifier registry in rules/sort-imports/types.ts (the array containing 'type','side-effect-style','side-effect','style','tsconfig-path','index','sibling','parent','subpath','internal','builtin','external') changes GroupMatcher's index-based precedence; add focused regression tests that assert exact matching priority for overlapping import cases (e.g., imports that could match both 'type' and 'tsconfig-path', 'style' and 'side-effect-style', or 'internal' vs 'external') to lock the new order, and duplicate the same targeted precedence tests for the other reordered block referenced around the second region (lines 174-180) so future reorderings will fail the tests if precedence changes.rules/sort-classes/types.ts (1)
133-143: Please pin the new class-member precedence with targeted overlap tests.This reorder is behavioral under GroupMatcher index-based comparison. Adding focused tests for overlapping matches (e.g., generic vs specialized selectors, multi-modifier matches) will protect the intended ordering semantics.
Also applies to: 149-161
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rules/sort-classes/types.ts` around lines 133 - 143, The new ordering in the class-member precedence list (allSelectors) introduces behavioral changes under GroupMatcher index-based comparison; add focused unit tests that pin this ordering by asserting overlap resolution between generic and specialized selectors and multi-modifier combinations. Create targeted tests that exercise overlapping cases (e.g., 'property' vs 'function-property', 'method' vs 'get-method'/'set-method', static vs instance matches) to ensure the GroupMatcher picks the intended selector by index; also add the same style of overlap tests for the second selector array in this module to cover both changed lists. Ensure tests are deterministic by constructing minimal class/member examples that match multiple selectors and asserting the expected selector is chosen.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rules/sort-decorators.ts`:
- Around line 98-155: The visitor calls to sortDecorators (in the Decorator
visitor and the PropertyDefinition, AccessorProperty, MethodDefinition,
ClassDeclaration handlers) do not pass the selector/modifier context, causing
the grouping logic to see selectors:[] and modifiers:[] and miss
selector/modifier-specific groups; update each call (including the Decorator
branch that handles parameter decorators and the calls that use
getNodeDecorators) to pass the node's selector (e.g., "parameter" for the
Decorator/parameter case, "property", "accessor", "method", "class" for the
respective handlers) and the node's modifier metadata
(static/readonly/visibility/etc.) to sortDecorators so the groupMatcher can
match selector- or modifier-scoped groups. Ensure the Decorator branch that
inspects decorator.parent/FunctionExpression uses the parameter selector and
that calls using getNodeDecorators forward the originating AST node or an
extracted { selector, modifiers } object into sortDecorators.
In `@rules/sort-modules/types.ts`:
- Around line 103-110: The array allModifiers was reordered and that changes the
predefined-group naming surface; revert the change by restoring the original
sequence so 'async' and 'decorated' remain in their previous positions (i.e.,
keep allModifiers as it was before the refactor) so existing groups/customGroups
string matches are preserved; ensure the exported type Modifier remains derived
from (typeof allModifiers)[number] and do not alter ordering unless you provide
an explicit migration path for groups/customGroups entries.
---
Nitpick comments:
In `@rules/sort-classes/types.ts`:
- Around line 133-143: The new ordering in the class-member precedence list
(allSelectors) introduces behavioral changes under GroupMatcher index-based
comparison; add focused unit tests that pin this ordering by asserting overlap
resolution between generic and specialized selectors and multi-modifier
combinations. Create targeted tests that exercise overlapping cases (e.g.,
'property' vs 'function-property', 'method' vs 'get-method'/'set-method', static
vs instance matches) to ensure the GroupMatcher picks the intended selector by
index; also add the same style of overlap tests for the second selector array in
this module to cover both changed lists. Ensure tests are deterministic by
constructing minimal class/member examples that match multiple selectors and
asserting the expected selector is chosen.
In `@rules/sort-imports/types.ts`:
- Around line 151-162: The reordered selector/modifier registry in
rules/sort-imports/types.ts (the array containing
'type','side-effect-style','side-effect','style','tsconfig-path','index','sibling','parent','subpath','internal','builtin','external')
changes GroupMatcher's index-based precedence; add focused regression tests that
assert exact matching priority for overlapping import cases (e.g., imports that
could match both 'type' and 'tsconfig-path', 'style' and 'side-effect-style', or
'internal' vs 'external') to lock the new order, and duplicate the same targeted
precedence tests for the other reordered block referenced around the second
region (lines 174-180) so future reorderings will fail the tests if precedence
changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e8fe8a24-9463-4f81-ab22-e8e27a749345
📒 Files selected for processing (45)
rules/sort-array-includes.tsrules/sort-arrays.tsrules/sort-arrays/sort-array.tsrules/sort-arrays/types.tsrules/sort-classes/sort-class.tsrules/sort-classes/types.tsrules/sort-decorators.tsrules/sort-decorators/types.tsrules/sort-enums/sort-enum.tsrules/sort-enums/types.tsrules/sort-exports.tsrules/sort-heritage-clauses/sort-heritage-clause.tsrules/sort-heritage-clauses/types.tsrules/sort-import-attributes/sort-import-or-export-attributes.tsrules/sort-import-attributes/types.tsrules/sort-imports.tsrules/sort-imports/types.tsrules/sort-intersection-types.tsrules/sort-jsx-props/sort-jsx-object.tsrules/sort-maps/sort-potential-map.tsrules/sort-maps/types.tsrules/sort-modules.tsrules/sort-modules/types.tsrules/sort-named-exports/sort-named-export.tsrules/sort-named-imports/sort-named-import.tsrules/sort-object-types/sort-object-type-elements.tsrules/sort-object-types/types.tsrules/sort-objects/sort-object.tsrules/sort-objects/types.tsrules/sort-sets.tsrules/sort-union-types.tsrules/sort-union-types/sort-union-or-intersection-types.tsrules/sort-union-types/types.tsrules/sort-variable-declarations/sort-variable-declaration.tsrules/sort-variable-declarations/types.tstest/utils/compute-group.test.tstest/utils/does-custom-group-match.test.tstest/utils/generate-predefined-groups.test.tstest/utils/get-array-combinations.test.tstest/utils/validate-groups-configuration.test.tsutils/compute-group.tsutils/does-custom-group-match.tsutils/generate-predefined-groups.tsutils/get-array-combinations.tsutils/group-matcher.ts
💤 Files with no reviewable changes (11)
- rules/sort-array-includes.ts
- rules/sort-union-types.ts
- rules/sort-sets.ts
- rules/sort-arrays.ts
- test/utils/generate-predefined-groups.test.ts
- rules/sort-intersection-types.ts
- test/utils/get-array-combinations.test.ts
- test/utils/compute-group.test.ts
- utils/get-array-combinations.ts
- utils/generate-predefined-groups.ts
- utils/compute-group.ts
Comment on lines
+98
to
+155
| return { | ||
| Decorator: decorator => { | ||
| if (!options.sortOnParameters) { | ||
| return | ||
| } | ||
| if ( | ||
| 'decorators' in decorator.parent && | ||
| decorator.parent.type === AST_NODE_TYPES.Identifier && | ||
| decorator.parent.parent.type === AST_NODE_TYPES.FunctionExpression | ||
| ) { | ||
| let { decorators } = decorator.parent | ||
| if (decorator !== decorators[0]) { | ||
| return | ||
| } | ||
| sortDecorators({ groupMatcher, decorators, options, context }) | ||
| } | ||
| }, | ||
| PropertyDefinition: propertyDefinition => { | ||
| if (options.sortOnProperties) { | ||
| sortDecorators({ | ||
| decorators: getNodeDecorators(propertyDefinition), | ||
| groupMatcher, | ||
| context, | ||
| options, | ||
| }) | ||
| } | ||
| }, | ||
| AccessorProperty: accessorDefinition => { | ||
| if (options.sortOnAccessors) { | ||
| sortDecorators({ | ||
| decorators: getNodeDecorators(accessorDefinition), | ||
| groupMatcher, | ||
| context, | ||
| options, | ||
| }) | ||
| } | ||
| }, | ||
| MethodDefinition: methodDefinition => { | ||
| if (options.sortOnMethods) { | ||
| sortDecorators({ | ||
| decorators: getNodeDecorators(methodDefinition), | ||
| groupMatcher, | ||
| context, | ||
| options, | ||
| }) | ||
| } | ||
| }, | ||
| ClassDeclaration: declaration => { | ||
| if (options.sortOnClasses) { | ||
| sortDecorators({ | ||
| decorators: getNodeDecorators(declaration), | ||
| groupMatcher, | ||
| context, | ||
| options, | ||
| }) | ||
| } | ||
| }, | ||
| } |
There was a problem hiding this comment.
Pass selector/modifier context into sortDecorators().
These call sites now drop whether the decorators belong to a parameter, property, accessor, method, or class, and the helper consequently computes every group with selectors: [] / modifiers: []. Any predefined or custom decorator group that filters by selector/modifier becomes unreachable and falls back to the generic group.
Also applies to: 226-264
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rules/sort-decorators.ts` around lines 98 - 155, The visitor calls to
sortDecorators (in the Decorator visitor and the PropertyDefinition,
AccessorProperty, MethodDefinition, ClassDeclaration handlers) do not pass the
selector/modifier context, causing the grouping logic to see selectors:[] and
modifiers:[] and miss selector/modifier-specific groups; update each call
(including the Decorator branch that handles parameter decorators and the calls
that use getNodeDecorators) to pass the node's selector (e.g., "parameter" for
the Decorator/parameter case, "property", "accessor", "method", "class" for the
respective handlers) and the node's modifier metadata
(static/readonly/visibility/etc.) to sortDecorators so the groupMatcher can
match selector- or modifier-scoped groups. Ensure the Decorator branch that
inspects decorator.parent/FunctionExpression uses the parameter selector and
that calls using getNodeDecorators forward the originating AST node or an
extracted { selector, modifiers } object into sortDecorators.
Comment on lines
103
to
+110
| export let allModifiers = [ | ||
| 'async', | ||
| 'declare', | ||
| 'decorated', | ||
| 'default', | ||
| 'async', | ||
| 'decorated', | ||
| 'export', | ||
| ] as const | ||
| export type Modifier = (typeof allModifiers)[number] |
There was a problem hiding this comment.
Keep the existing sort-modules modifier order.
allModifiers is part of the predefined-group naming surface, so reordering 'async' / 'decorated' changes which strings existing groups / customGroups entries match. That turns this refactor into a config-breaking change unless the previous order is preserved or explicitly migrated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rules/sort-modules/types.ts` around lines 103 - 110, The array allModifiers
was reordered and that changes the predefined-group naming surface; revert the
change by restoring the original sequence so 'async' and 'decorated' remain in
their previous positions (i.e., keep allModifiers as it was before the refactor)
so existing groups/customGroups string matches are preserved; ensure the
exported type Modifier remains derived from (typeof allModifiers)[number] and do
not alter ordering unless you provide an explicit migration path for
groups/customGroups entries.
hugop95
force-pushed
the
refactor/groups
branch
4 times, most recently
from
82484a7 to
85c2b2e
Compare
hugop95
force-pushed
the
refactor/groups
branch
2 times, most recently
from
d7346e4 to
edc957e
Compare
hugop95
force-pushed
the
refactor/groups
branch
2 times, most recently
from
8f07363 to
94dadc3
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
oxlint's implementation of group matching is technically a bit more elegant than the current one.
The current approach, in theory, is opening a known and accepted possibility for permutation explosion. Performance impact was mitigated through caching.
eslint-plugin-perfectionist/utils/generate-predefined-groups.ts
Lines 54 to 66 in b98c82d
In practice, this was very unlikely/impossible due to the number of possible modifiers.