fix(sort-modules): add missing partition cases#728
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdded exhaustive type-guarding in module AST handling and expanded which AST node types are treated as non-sortable or as sorting-group boundaries; tests extended to cover many statement forms and TypeScript-specific declarations affecting partitioning. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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. 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 |
| break | ||
| /* v8 ignore next 2 -- @preserve Exhaustive guard. */ | ||
| default: | ||
| assertIsNever(node) |
There was a problem hiding this comment.
reviewers
I'm not doing throw new UnreachableCaseError to preserve current behavior in case custom parsers return other types.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #728 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 223 223
Lines 4364 4385 +21
Branches 1353 1374 +21
=========================================
+ Hits 4364 4385 +21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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 `@test/rules/sort-modules.test.ts`:
- Around line 409-418: Update the tests so they actually assert the partition
behavior: in the "creates partitions at TypeScript import-equals declarations"
case (and the related cases between lines 431-483) replace the neutral ordering
with reversed names so sorting must move declarations across the import-equals
boundary (e.g., use a B … A ordering where A should sort before B), and convert
the "sorts across …" tests to use invalid(...) assertions that verify the
rule-fixer reorders the sortable declarations but leaves the import-equals (the
skipped node) in place; adjust the test inputs and expectations for the
valid/invalid helpers (functions valid and invalid) accordingly so they fail
when TSImportEqualsDeclaration is not treated as a partition.
🪄 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: 58724648-7a0e-43da-9db9-4e560c93c6d6
📒 Files selected for processing (2)
rules/sort-modules.tstest/rules/sort-modules.test.ts
d82e539 to
7cec264
Compare
| case AST_NODE_TYPES.ImportDeclaration: | ||
| case AST_NODE_TYPES.DebuggerStatement: | ||
| case AST_NODE_TYPES.ContinueStatement: | ||
| /* v8 ignore next 4 -- @preserve These statements cannot appear at module/namespace level in valid code. */ |
There was a problem hiding this comment.
Is this v8 ignore still correct? EmptyStatement is valid at top level (;), so the comment seems too broad.
There was a problem hiding this comment.
It was from an early implementation, we are testing everything now, it's useless 👍
| `, | ||
| options: [options], | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Could these sorts across ... cases be turned into invalid(...) tests with B ... skipped node ... A?
Right now they use already sorted input, so they still pass even if those skipped nodes accidentally become partition boundaries. Coverage is green, but I don't think those tests actually pin the behavior down yet.
There was a problem hiding this comment.
Description
sort-modulesdoesn't handle several AST node types that can appear at module or namespace level and ignore them. In theory, this can create issues (see the tests).This PR adds the following list of new node types that now trigger partitioning:
TSImportEqualsDeclarationTSExportAssignmentDoWhileStatementLabeledStatementSwitchStatementWhileStatementForInStatementForOfStatementThrowStatementBlockStatementForStatementTryStatementIfStatement