-
-
Notifications
You must be signed in to change notification settings - Fork 660
Add linting to validate type annotations in twoslash (// =>) JSDoc comments
#1309
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
PR ReviewThis PR adds automated validation of twoslash type comments in JSDoc code blocks. While the feature is valuable, there are several issues that need to be addressed: Critical Issues1. Typo in lint rule (line 131) - Should be "environment", not "enviroment". 2. Disabled ESLint rules without proper justification - Lines 1-2 disable complexity and max-depth rules. Per CLAUDE.md: "Fix lint issues, don't disable rules." The code should be refactored to reduce complexity instead of disabling these rules. Consider extracting helper functions. 3. Incorrect example in source/remove-prefix.d.ts - Lines 22-24 show an unrelated example in the RemovePrefixOptions.strict documentation. This example doesn't demonstrate the strict option at all - it's just showing object type inference, which is completely irrelevant to prefix removal. 4. Union type order instability - source/array-element.d.ts line 23 changed from "1 | 2 | 3" to "3 | 1 | 2". Union type member order shouldn't matter semantically, but if the linter enforces a specific order, this indicates the type inference is non-deterministic or the linter needs better normalization. Code Quality Concerns5. Deep nesting in type validation logic - Lines 166-279 contain heavily nested loops (up to 4-5 levels deep). This makes the code hard to test and maintain. Extract functions like findTwoslashComments, extractQuickInfoType, formatExpectedComment. 6. Magic number without explanation - Line 236: if (expectedType.length < 80) - Why 80? Add a named constant like SINGLE_LINE_TYPE_THRESHOLD. 7. Insufficient error handling - Line 56-58: Silent fallback when JSON parsing fails. Should at least log a warning for malformed compiler options. 8. parseCompilerOptions stops at first non-match - Lines 40-49: The function stops parsing compiler options at the first non-matching line. This means empty lines between options or non-option comments will prevent later options from being read. Is this intentional? Needs documentation or fix. Testing Concerns9. WIP status - PR body says "WIP" - this needs to be removed when ready for review. 10. Massive test changes - 572 additions to test file. While comprehensive, ensure all edge cases are covered: generic types with multiple type parameters, conditional types, mapped types with complex keys. Minor Issues11. Inconsistent variable naming - Line 127: env is reassigned - use const currentEnv or similar to avoid reassignment. Recommendations
The core feature is valuable, but the implementation needs refinement before merge. |
1099024 to
95ce69a
Compare
WIP
TL;DR

Catches issues like this.