-
Notifications
You must be signed in to change notification settings - Fork 189
fix: type generation for enums by prefixing table name #1249
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
Conversation
WalkthroughThis pull request updates type-generation templates for C#, Dart, Java, Kotlin, PHP, Swift, and TypeScript to thread collection context into type resolution. The Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Areas requiring extra attention:
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
templates/cli/lib/type-generation/languages/csharp.js.twig(4 hunks)templates/cli/lib/type-generation/languages/dart.js.twig(4 hunks)templates/cli/lib/type-generation/languages/java.js.twig(4 hunks)templates/cli/lib/type-generation/languages/kotlin.js.twig(3 hunks)templates/cli/lib/type-generation/languages/php.js.twig(4 hunks)templates/cli/lib/type-generation/languages/swift.js.twig(6 hunks)templates/cli/lib/type-generation/languages/typescript.js.twig(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: build (8.3, Ruby31)
- GitHub Check: build (8.3, AppleSwift56)
- GitHub Check: build (8.3, WebChromium)
- GitHub Check: build (8.3, Python311)
- GitHub Check: dotnet (server)
- GitHub Check: build (8.3, Python310)
- GitHub Check: build (8.3, Ruby27)
- GitHub Check: swift (server)
- GitHub Check: kotlin (server)
- GitHub Check: build (8.3, PHP80)
- GitHub Check: react-native (client)
- GitHub Check: android (client)
- GitHub Check: web (client)
- GitHub Check: build (8.3, FlutterStable)
- GitHub Check: build (8.3, FlutterBeta)
- GitHub Check: apple (client)
- GitHub Check: build (8.3, Android5Java17)
- GitHub Check: build (8.3, CLINode18)
- GitHub Check: build (8.3, Android14Java17)
- GitHub Check: build (8.3, DartStable)
🔇 Additional comments (15)
templates/cli/lib/type-generation/languages/kotlin.js.twig (1)
6-75: Enum prefix propagation looks clean. The extracollectionNameparameter flows through to each call site, so Kotlin enums and property types now emit collection-scoped identifiers without leaving any stragglers.templates/cli/lib/type-generation/languages/swift.js.twig (1)
6-147: Swift template updates are consistent. Prefixed enum identifiers feed through getType, the enum declaration, and both decode/from-map paths, so collisions across collections are avoided.templates/cli/lib/type-generation/languages/typescript.js.twig (1)
9-95: TypeScript typing is scoped correctly. Passingcollection.nameintogetTypeensures both enum declarations and property types share the new collection-prefixed identifier, solving the previous collision issue.templates/cli/lib/type-generation/languages/csharp.js.twig (1)
6-99: C# enum handling aligns with the new scheme. The prefixed names flow fromgetTypethrough property signatures andEnum.Parse, so generated models won’t clash across collections anymore.templates/cli/lib/type-generation/languages/dart.js.twig (1)
43-177: Dart enum renaming is thorough. Prefixed enums are emitted and consumed consistently in fields, constructors, and map conversions, so name clashes across collections are resolved end-to-end.templates/cli/lib/type-generation/languages/java.js.twig (5)
6-15: LGTM! Enum type generation now includes collection context.The method signature correctly adds the
collectionNameparameter, and enum types are properly prefixed with the collection name. This change prevents naming conflicts when multiple collections have attributes with the same enum name.Note: This is a breaking change. Existing generated code that references enum types by their attribute name alone will need to be updated to use the new prefixed names (e.g.,
AttributeName→CollectionNameAttributeName).
64-68: LGTM! Enum declaration consistent with type generation.The enum declaration correctly includes the collection name prefix, matching the type generation logic in the
getTypemethod.
73-73: LGTM! Field declarations updated correctly.The private field declarations correctly pass
collection.nameto thegetTypemethod.
79-87: LGTM! Constructor parameters updated correctly.Constructor parameter types correctly use
getType(attribute, collections, collection.name), ensuring consistency with field declarations.
90-96: LGTM! Getter and setter methods updated correctly.Both getter return types and setter parameter types correctly pass
collection.nameto thegetTypemethod, ensuring type consistency across the generated class.templates/cli/lib/type-generation/languages/php.js.twig (5)
6-19: LGTM! Enum type generation now includes collection context.The method signature correctly adds the
collectionNameparameter, and enum types are properly prefixed with the collection name, consistent with the Java implementation.Note: This is a breaking change. Existing generated code that references enum types by their attribute name alone will need to be updated to use the new prefixed names.
63-68: LGTM! Enum declaration consistent with type generation.The enum declaration correctly includes the collection name prefix and uses PHP 8.1+ backed enum syntax with appropriate case naming conventions.
73-73: LGTM! Property declarations updated correctly.The private property declarations correctly pass
collection.nameto thegetTypemethod.
76-88: LGTM! Constructor parameters updated correctly.Constructor parameter types correctly use
getType(attribute, collections, collection.name)with appropriate handling of nullable types for PHP's parameter declaration syntax.
91-97: LGTM! Getter and setter methods updated correctly.Both getter return types and setter parameter types correctly pass
collection.nameto thegetTypemethod, ensuring type consistency across the generated class.
Summary by CodeRabbit
Bug Fixes
Improvements