-
Notifications
You must be signed in to change notification settings - Fork 53
fix(parsePath): not throwing error when package #988
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
Tagging OptionsShould a new tag be published when this PR is merged?
|
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant parsePath
participant resolveFileExtension
rect rgb(200, 220, 240)
Note over parsePath: Old Flow
Caller->>parsePath: parsePath(filePath)
alt path starts with "file:"
parsePath->>resolveFileExtension: resolve extension
resolveFileExtension-->>parsePath: unresolved
parsePath->>parsePath: continue with unresolved path<br/>(potential errors)
end
parsePath-->>Caller: result or error
end
rect rgb(220, 240, 220)
Note over parsePath: New Flow
Caller->>parsePath: parsePath(filePath)
alt path starts with "file:"
parsePath->>resolveFileExtension: resolve extension
resolveFileExtension-->>parsePath: unresolved
alt no recognized extension
parsePath->>parsePath: console.warn()
parsePath-->>Caller: undefined
else has extension
parsePath->>parsePath: proceed as before
end
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
engine/schema/parser.ts (1)
303-310: Consider extracting the duplicated extension regex pattern.The regex pattern
/\.(ts|tsx|js|jsx|mjs|cjs|d\.ts|d\.mts|d\.cts)$/is duplicated from line 65. Consider extracting it to a named constant at the module level to improve maintainability and ensure consistency if the list of supported extensions ever changes.For example, add near the top of the file:
+const KNOWN_EXTENSIONS_REGEX = /\.(ts|tsx|js|jsx|mjs|cjs|d\.ts|d\.mts|d\.cts)$/; + const JS_REGEX_PATH: RegExp = /\.(m?js|cjs)$/;Then update both occurrences:
// In resolveFileExtension (line 65) - if (pathStr.match(/\.(ts|tsx|js|jsx|mjs|cjs|d\.ts|d\.mts|d\.cts)$/)) { + if (pathStr.match(KNOWN_EXTENSIONS_REGEX)) { return filePath; }// In parsePath (line 303) - } else if (!path.match(/\.(ts|tsx|js|jsx|mjs|cjs|d\.ts|d\.mts|d\.cts)$/)) { + } else if (!path.match(KNOWN_EXTENSIONS_REGEX)) { // If resolution failed and path has no extension, return undefined instead of throwing
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
engine/schema/parser.ts(1 hunks)
🔇 Additional comments (1)
engine/schema/parser.ts (1)
303-310: Good improvement: graceful handling of unresolvable paths.This change appropriately handles the case where a file path cannot be resolved and lacks a recognized extension. Returning
undefinedearly with a warning is consistent with the npm package handling pattern (lines 289-294) and prevents unnecessary attempts to load non-existent files that would fail later at line 317.The warning message provides helpful debugging information while avoiding exceptions for paths that cannot be resolved.
Summary by CodeRabbit