fix: add Oxlint rule tester and fix issue in sort-classes#726
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds an oxlint-based RuleTester and oxlint test cases across many sorting rule suites, introduces an exported compile-time exhaustive-check helper Changes
Sequence Diagram(s)sequenceDiagram
participant TestFile as Test Suite
participant Builder as buildOxlintRuleTester
participant RuleTester as oxlint RuleTester
participant RuleModule as Rule
participant Vitest as Vitest/it
TestFile->>Builder: import & call buildOxlintRuleTester(rule, {lang})
Builder->>RuleTester: configure (languageOptions, parserOptions)
Builder->>Vitest: set RuleTester.it = it
TestFile->>RuleTester: run(name, tests)
RuleTester->>RuleModule: instantiate with languageOptions
RuleTester->>RuleModule: execute rule against fixtures
RuleModule-->>RuleTester: report diagnostics/fixes
RuleTester-->>TestFile: surface test results/assertions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
|
| 📦 Package | 📋 Versions |
|---|---|
| @types/node | 2 versions
|
| @types/unist | 2 versions
|
| changelogen | 2 versions
|
| tinyexec | 2 versions
|
| typescript | 2 versions
|
| unist-util-visit | 2 versions
|
| chokidar | 3 versions
|
| picomatch | 2 versions
|
| @astrojs/compiler | 2 versions
|
| unist-util-visit-parents | 2 versions
|
| source-map | 2 versions
|
| zod | 2 versions
|
| yaml | 3 versions
|
| globals | 3 versions
|
| js-tokens | 3 versions
|
| semver | 3 versions
|
| lru-cache | 3 versions
|
| keyv | 2 versions
|
| ajv | 2 versions
|
| global-directory | 2 versions
|
| resolve-from | 2 versions
|
| css-tree | 2 versions
|
| escape-string-regexp | 3 versions
|
| ignore | 2 versions
|
| eslint-visitor-keys | 3 versions
|
| minimatch | 4 versions
|
| estree-walker | 2 versions
|
| @rollup/pluginutils | 2 versions
|
| strip-json-comments | 2 versions
|
| supports-color | 3 versions
|
| argparse | 2 versions
|
| undici-types | 2 versions
|
| std-env | 2 versions
|
| pathe | 2 versions
|
| request-light | 2 versions
|
| jsonc-parser | 2 versions
|
| entities | 3 versions
|
| json-schema-traverse | 2 versions
|
| eslint-scope | 2 versions
|
| espree | 2 versions
|
| aria-query | 2 versions
|
| cookie | 2 versions
|
| html-escaper | 2 versions
|
| p-limit | 2 versions
|
| yargs-parser | 2 versions
|
| brace-expansion | 3 versions
|
| balanced-match | 2 versions
|
| c12 | 2 versions
|
| confbox | 2 versions
|
| dotenv | 2 versions
|
| giget | 2 versions
|
| jiti | 2 versions
|
| ohash | 2 versions
|
| perfect-debounce | 2 versions
|
| pkg-types | 2 versions
|
| glob-parent | 2 versions
|
| readdirp | 3 versions
|
| citty | 2 versions
|
| lilconfig | 2 versions
|
| parent-module | 2 versions
|
| string-width | 2 versions
|
| strip-ansi | 2 versions
|
| meow | 2 versions
|
| env-paths | 2 versions
|
| path-key | 2 versions
|
| which | 2 versions
|
| ansi-regex | 2 versions
|
| commander | 2 versions
|
| mdn-data | 2 versions
|
| eslint-compat-utils | 2 versions
|
| spdx-expression-parse | 2 versions
|
| emoji-regex | 2 versions
|
| zod-validation-error | 2 versions
|
| builtin-modules | 2 versions
|
| postcss-safe-parser | 2 versions
|
| file-entry-cache | 2 versions
|
| flat-cache | 2 versions
|
| minipass | 2 versions
|
| nypm | 2 versions
|
| ini | 3 versions
|
| unicorn-magic | 2 versions
|
| unist-util-find-after | 2 versions
|
| yallist | 2 versions
|
| unist-util-is | 2 versions
|
| yocto-queue | 2 versions
|
| has-flag | 2 versions
|
💡 To find out what depends on a specific package, run: pnpm -r why example-package
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #726 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 221 223 +2
Lines 4352 4364 +12
Branches 1350 1353 +3
=========================================
+ Hits 4352 4364 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
hugop95
changed the title
sort-classes
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rules/sort-classes/node-info/common-modifiers.ts (1)
45-55:⚠️ Potential issue | 🟠 MajorNormalize
nullaccessibility to'public'instead of dropping the modifier.The current fallback returns
[], which removes accessibility from computed modifiers and can mis-group/sort members. For oxlint’snull, this should behave like missing accessibility (public), not “no modifier”.💡 Proposed fix
export function computeAccessibilityModifier({ hasPrivateHash, node, }: { node: Accessor | Property | Method hasPrivateHash: boolean }): Modifier[] { if (hasPrivateHash) { return ['private'] } - switch (node.accessibility) { + const accessibility = node.accessibility ?? 'public' + + switch (accessibility) { case 'protected': return ['protected'] case 'private': return ['private'] - case undefined: case 'public': return ['public'] default: - assertIsNever(node.accessibility) - return [] + assertIsNever(accessibility) + return ['public'] } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rules/sort-classes/node-info/common-modifiers.ts` around lines 45 - 55, The switch over node.accessibility currently drops null by falling into default and returning [], which removes the accessibility modifier; update the handling in the switch (the one switching on node.accessibility) so that null is treated the same as undefined/'public' (either add an explicit case null that returns ['public'] or coerce node.accessibility to 'public' before the switch), keeping the assertIsNever(node.accessibility) in place for true unexpected values.
🧹 Nitpick comments (2)
test/rules/sort-heritage-clauses.test.ts (1)
3287-3294: Consider adding one valid class case in Oxlint coverage.You already validate class behavior in
invalid; adding a sorted class case invalidwould close the false-positive gap for that node type.Suggested diff
valid: [ { code: dedent` interface Interface extends a, b {} `, options: [{ type: 'alphabetical', order: 'asc' }], }, + { + code: dedent` + class Class implements a, b {} + `, + options: [{ type: 'alphabetical', order: 'asc' }], + }, ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/rules/sort-heritage-clauses.test.ts` around lines 3287 - 3294, Add a positive test for a sorted class heritage clause to the existing valid cases so class nodes are covered (mirror the interface case). In the test file sort-heritage-clauses.test.ts add an entry to the valid array with a class declaration that has sorted extends/implements (e.g., "class MyClass extends A, B {}" or includes implements) and use the same options object used by the interface case (options: [{ type: 'alphabetical', order: 'asc' }]) so the rule's class behavior is validated as a true-positive.test/rules/sort-decorators.test.ts (1)
6608-6615: Optional: broaden Oxlintvalidcoverage to member contexts.Current Oxlint
validonly checks top-level class decorators. Adding one sorted member/accessor/method/parameter case would better guard against backend-specific false positives.Suggested diff
valid: [ { code: dedent` `@A` `@B` class Foo {} `, options: [{ type: 'alphabetical', order: 'asc' }], }, + { + code: dedent` + class Foo { + `@A` + `@B` + property + + `@A` + `@B` + accessor field + + `@A` + `@B` + method( + `@A` + `@B` + parameter) {} + } + `, + options: [{ type: 'alphabetical', order: 'asc' }], + }, ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/rules/sort-decorators.test.ts` around lines 6608 - 6615, The test suite's `valid` cases only cover top-level class decorators; add at least one sorted-member case to guard against backend-specific false positives by adding examples of decorators in member/accessor/method/parameter contexts (e.g., a class property with `@A/`@B, a getter/setter or method decorated with `@A/`@B, and a parameter decorated with `@A/`@B) using the same options ({ type: 'alphabetical', order: 'asc' }) so the rule is exercised for non-top-level decorator positions; update the `valid` array in test/rules/sort-decorators.test.ts to include these strings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/rules/build-oxlint-rule-tester.ts`:
- Line 17: The parameter default only applies when the whole param is omitted;
update buildOxlintRuleTester's parameter destructuring so the lang property
itself defaults to 'ts' (i.e., change the signature from "{ lang }: { lang?:
Language } = { lang: 'ts' }" to using a property default and an empty-object
default for the param) so calling buildOxlintRuleTester(rule, {}) yields lang
=== 'ts'.
---
Outside diff comments:
In `@rules/sort-classes/node-info/common-modifiers.ts`:
- Around line 45-55: The switch over node.accessibility currently drops null by
falling into default and returning [], which removes the accessibility modifier;
update the handling in the switch (the one switching on node.accessibility) so
that null is treated the same as undefined/'public' (either add an explicit case
null that returns ['public'] or coerce node.accessibility to 'public' before the
switch), keeping the assertIsNever(node.accessibility) in place for true
unexpected values.
---
Nitpick comments:
In `@test/rules/sort-decorators.test.ts`:
- Around line 6608-6615: The test suite's `valid` cases only cover top-level
class decorators; add at least one sorted-member case to guard against
backend-specific false positives by adding examples of decorators in
member/accessor/method/parameter contexts (e.g., a class property with `@A/`@B, a
getter/setter or method decorated with `@A/`@B, and a parameter decorated with
`@A/`@B) using the same options ({ type: 'alphabetical', order: 'asc' }) so the
rule is exercised for non-top-level decorator positions; update the `valid`
array in test/rules/sort-decorators.test.ts to include these strings.
In `@test/rules/sort-heritage-clauses.test.ts`:
- Around line 3287-3294: Add a positive test for a sorted class heritage clause
to the existing valid cases so class nodes are covered (mirror the interface
case). In the test file sort-heritage-clauses.test.ts add an entry to the valid
array with a class declaration that has sorted extends/implements (e.g., "class
MyClass extends A, B {}" or includes implements) and use the same options object
used by the interface case (options: [{ type: 'alphabetical', order: 'asc' }])
so the rule's class behavior is validated as a true-positive.
🪄 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: 7de94fda-cef0-41ca-9115-33e61b4ed001
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!pnpm-lock.yaml
📒 Files selected for processing (28)
cspell.config.tspackage.jsonrules/sort-classes/is-known-class-element.tsrules/sort-classes/node-info/common-modifiers.tstest/rules/build-oxlint-rule-tester.tstest/rules/sort-array-includes.test.tstest/rules/sort-classes.test.tstest/rules/sort-decorators.test.tstest/rules/sort-enums.test.tstest/rules/sort-export-attributes.test.tstest/rules/sort-exports.test.tstest/rules/sort-heritage-clauses.test.tstest/rules/sort-import-attributes.test.tstest/rules/sort-imports.test.tstest/rules/sort-interfaces.test.tstest/rules/sort-intersection-types.test.tstest/rules/sort-jsx-props.test.tstest/rules/sort-maps.test.tstest/rules/sort-modules.test.tstest/rules/sort-named-exports.test.tstest/rules/sort-named-imports.test.tstest/rules/sort-object-types.test.tstest/rules/sort-objects.test.tstest/rules/sort-sets.test.tstest/rules/sort-switch-case.test.tstest/rules/sort-union-types.test.tstest/rules/sort-variable-declarations.test.tsutils/assert-is-never.ts
| */ | ||
| export function buildOxlintRuleTester( | ||
| rule: RuleModule, | ||
| { lang }: { lang?: Language } = { lang: 'ts' }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether any current callsites pass an empty options object or undefined lang.
rg -nP --type ts '\bbuildOxlintRuleTester\s*\(\s*[^,]+,\s*\{\s*\}\s*\)'
rg -nP --type ts '\bbuildOxlintRuleTester\s*\(\s*[^,]+,\s*\{\s*lang\s*:\s*undefined\s*\}\s*\)'Repository: azat-io/eslint-plugin-perfectionist
Length of output: 61
Default lang parameter should default the property, not just the entire parameter.
Line 17's destructuring pattern only defaults when the parameter is omitted entirely. Calling buildOxlintRuleTester(rule, {}) would pass lang: undefined despite the documented default being 'ts'. While no current callsites trigger this, the fix improves API correctness and defensiveness:
Proposed fix
export function buildOxlintRuleTester(
rule: RuleModule,
- { lang }: { lang?: Language } = { lang: 'ts' },
+ { lang = 'ts' }: { lang?: Language } = {},
): {
run(name: string, tests: RuleTester.TestCases): void
} {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| { lang }: { lang?: Language } = { lang: 'ts' }, | |
| { lang = 'ts' }: { lang?: Language } = {}, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/rules/build-oxlint-rule-tester.ts` at line 17, The parameter default
only applies when the whole param is omitted; update buildOxlintRuleTester's
parameter destructuring so the lang property itself defaults to 'ts' (i.e.,
change the signature from "{ lang }: { lang?: Language } = { lang: 'ts' }" to
using a property default and an empty-object default for the param) so calling
buildOxlintRuleTester(rule, {}) yields lang === 'ts'.
- This requires a specific language option.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-classes/node-info/common-modifiers.ts`:
- Around line 48-49: The current runtime throw discards the modifier because
Error only takes one message argument; update the throw in common-modifiers.ts
(the branch that currently does: throw new Error('Unhandled accessibility
modifier', modifier)) to include the actual modifier value in the error message
(e.g., build a single message that embeds or stringifies the modifier) so
runtime errors preserve the unexpected AST value for debugging; ensure you keep
the existing "v8 ignore next -- `@preserve` Unhandled case" comment and reference
the same modifier variable when constructing the message.
🪄 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: 63d2f6ef-90a9-4628-baad-7f4ee7025e02
📒 Files selected for processing (26)
cspell.config.tsrules/sort-classes/is-known-class-element.tsrules/sort-classes/node-info/common-modifiers.tstest/rules/sort-array-includes.test.tstest/rules/sort-classes.test.tstest/rules/sort-decorators.test.tstest/rules/sort-enums.test.tstest/rules/sort-export-attributes.test.tstest/rules/sort-exports.test.tstest/rules/sort-heritage-clauses.test.tstest/rules/sort-import-attributes.test.tstest/rules/sort-imports.test.tstest/rules/sort-interfaces.test.tstest/rules/sort-intersection-types.test.tstest/rules/sort-jsx-props.test.tstest/rules/sort-maps.test.tstest/rules/sort-modules.test.tstest/rules/sort-named-exports.test.tstest/rules/sort-named-imports.test.tstest/rules/sort-object-types.test.tstest/rules/sort-objects.test.tstest/rules/sort-sets.test.tstest/rules/sort-switch-case.test.tstest/rules/sort-union-types.test.tstest/rules/sort-variable-declarations.test.tsutils/assert-is-never.ts
✅ Files skipped from review due to trivial changes (7)
- cspell.config.ts
- utils/assert-is-never.ts
- test/rules/sort-heritage-clauses.test.ts
- test/rules/sort-switch-case.test.ts
- test/rules/sort-maps.test.ts
- test/rules/sort-sets.test.ts
- test/rules/sort-modules.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- rules/sort-classes/is-known-class-element.ts
- test/rules/sort-export-attributes.test.ts
- test/rules/sort-interfaces.test.ts
- test/rules/sort-object-types.test.ts
- test/rules/sort-named-exports.test.ts
- test/rules/sort-classes.test.ts
- test/rules/sort-named-imports.test.ts
Comment on lines
+48
to
+49
| /* v8 ignore next -- @preserve Unhandled case */ | ||
| throw new Error('Unhandled accessibility modifier', modifier) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n "rules/sort-classes/node-info/common-modifiers.ts" | head -60Repository: azat-io/eslint-plugin-perfectionist
Length of output: 2208
🏁 Script executed:
cat -n "utils/assert-is-never.ts"Repository: azat-io/eslint-plugin-perfectionist
Length of output: 446
🏁 Script executed:
node -e "
try {
throw new Error('Test message', 'extra param');
} catch (e) {
console.log('Error message:', e.message);
console.log('Full error:', e.toString());
console.log('Error has 2 args, does second appear in message?', e.message.includes('extra param'));
}
"Repository: azat-io/eslint-plugin-perfectionist
Length of output: 193
Preserve the unexpected modifier in the runtime error.
The Error constructor does not include the second argument in the error message. Line 49's throw new Error('Unhandled accessibility modifier', modifier) silently discards the modifier value, making it impossible to diagnose future AST-shape mismatches at runtime. Since assertIsNever is compile-time only, this runtime path needs explicit error context.
Suggested change
- throw new Error('Unhandled accessibility modifier', modifier)
+ throw new Error(
+ `Unhandled accessibility modifier: ${String(modifier)}`,
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /* v8 ignore next -- @preserve Unhandled case */ | |
| throw new Error('Unhandled accessibility modifier', modifier) | |
| /* v8 ignore next -- `@preserve` Unhandled case */ | |
| throw new Error( | |
| `Unhandled accessibility modifier: ${String(modifier)}`, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rules/sort-classes/node-info/common-modifiers.ts` around lines 48 - 49, The
current runtime throw discards the modifier because Error only takes one message
argument; update the throw in common-modifiers.ts (the branch that currently
does: throw new Error('Unhandled accessibility modifier', modifier)) to include
the actual modifier value in the error message (e.g., build a single message
that embeds or stringifies the modifier) so runtime errors preserve the
unexpected AST value for debugging; ensure you keep the existing "v8 ignore next
-- `@preserve` Unhandled case" comment and reference the same modifier variable
when constructing the message.
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
2 participants
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 provides a different AST shape than ESLint. Notably, accessibility modifiers on class members can be
nullrather thanundefined, causing uncaught errors.Because Oxlint is gaining traction as an ESLint replacement/co-existing tool, ensuring compatibility with it will become increasingly important over time. As such, this PR lays the groundwork for reliable Oxlint support by introducing a dedicated Oxlint rule tester.