Detect duplicate declaration #9
Conversation
WalkthroughThe pull request introduces enhancements to the schema validation mechanism in the Yorkie project. The changes focus on improving error handling and type management within the Changes
Assessment against linked issues
Possibly related PRs
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (4)
src/validator.ts (3)
41-51: Consider enhancing the Diagnostic type documentation.The severity levels ('error' | 'warning' | 'info') could benefit from documentation explaining when each level should be used. This would help maintain consistency in error reporting across the codebase.
/** * `Diagnostic` represents a diagnostic message. + * + * Severity levels: + * - error: Used for issues that prevent correct schema validation + * - warning: Used for potential issues that don't invalidate the schema + * - info: Used for suggestions and best practices */ export type Diagnostic = {
65-71: Enhance duplicate type error message with original declaration location.Consider storing and including the location of the original declaration in the error message to help users quickly identify and fix duplicate declarations.
if (this.symbolMap.has(typeName)) { + const original = this.symbolMap.get(typeName)!; this.errors.push({ - message: `Duplicate type declaration: ${typeName}`, + message: `Duplicate type declaration: ${typeName} (originally declared at line ${original.line}, column ${original.column})`, line: line, column: charPositionInLine, }); }
Line range hint
194-220: Optimize circular reference detection algorithm.The current implementation has O(n²) complexity. Consider using a more efficient graph algorithm like Tarjan's strongly connected components algorithm for detecting cycles.
Would you like me to provide an optimized implementation using Tarjan's algorithm?
test/schema.test.ts (1)
101-119: Enhance test coverage with additional edge cases.Consider adding test cases for:
- Nested objects with duplicate properties
- Duplicate properties with different case (case sensitivity)
- Properties duplicated across inherited types
Example additional test cases:
it('should detect duplicate keys in nested objects', () => { const schema = ` type Document = { nested: { field: string; field: number; }; }; `; expect(validate(schema).errors.length).toBeGreaterThan(0); }); it('should handle case sensitivity in property names', () => { const schema = ` type Document = { field: string; Field: number; // Should this be considered a duplicate? }; `; expect(validate(schema).errors.length).toBeGreaterThan(0); });
hackerwins
force-pushed
the
detect-duplication
branch
from
a2b3dfb to
db4b9e0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/validator.ts (2)
22-51: Consider making type properties readonly for immutability.The new types are well-structured, but making their properties readonly would prevent accidental modifications and improve type safety.
type TypeSymbol = { - name: string; - line: number; - column: number; + readonly name: string; + readonly line: number; + readonly column: number; }; type TypeReference = { - name: string; - parent: string; - line: number; - column: number; + readonly name: string; + readonly parent: string; + readonly line: number; + readonly column: number; }; export type Diagnostic = { - severity: 'error' | 'warning' | 'info'; - message: string; - range: { - start: { column: number; line: number }; - end: { column: number; line: number }; - }; + readonly severity: 'error' | 'warning' | 'info'; + readonly message: string; + readonly range: { + readonly start: { readonly column: number; readonly line: number }; + readonly end: { readonly column: number; readonly line: number }; + }; };
Line range hint
185-191: Enhance error message for undefined type references.The error message could be more descriptive by including the parent type where the reference was used.
if (!listener.symbolMap.has(ref.name)) { listener.errors.push({ - message: `Type '${ref.name}' is not defined.`, + message: `Type '${ref.name}' referenced in '${ref.parent}' is not defined.`, line: ref.line, column: ref.column, }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/validator.ts(4 hunks)test/schema.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/schema.test.ts
🔇 Additional comments (2)
src/validator.ts (2)
11-14: LGTM!The addition of
PropertyNameContextimport is necessary for implementing the duplicate property detection feature.
Line range hint
195-224: Consider optimizing circular reference detection.The current implementation uses a while loop with Set operations. Consider using a more efficient graph-based algorithm for larger schemas.
Let's check if there are any large schema files in the codebase that might benefit from optimization:
What this PR does / why we need it:
Detect duplicate declaration
Which issue(s) this PR fixes:
Addresses yorkie-team/yorkie#971
Summary by CodeRabbit
New Features
Tests