CLI Dart: model generation fixes #1197
Conversation
WalkthroughThe Dart template switches to a centralized attribute list ___attrs (derived from strict mode) and replaces all collection.attributes references with __attrs, including length checks. Attributes are ordered required-first when strict is true; otherwise, original order is used. Relationship imports are deduplicated via __relatedImportsSeen. Field, constructor, fromMap, and toMap generation now iterate over __attrs. fromMap/toMap logic is refactored: enum handling is adjusted; array/nullability behavior is revised; relationship mapping applies element fromMap where applicable. Map key quotes in toMap are changed to single quotes. Comma/separator handling aligns with __attrs.length. Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
![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 (3)
templates/cli/lib/type-generation/languages/dart.js.twig (3)
108-108: Consider adding trailing comma consistency for better diff management.Currently, the enum generation conditionally adds commas. Adding a trailing comma after the last element would improve git diffs when new enum values are added.
Apply this diff to add consistent trailing commas:
- <%- strict ? toCamelCase(element) : element %><% if (index < attribute.elements.length - 1) { -%>,<% } %> + <%- strict ? toCamelCase(element) : element %>,
131-131: Potential null reference error in enum array mapping.When
map['<%= attribute.key %>']is null, the optional chaining?.map()will return null, but there's no null fallback like other array types have. This could cause issues if the consuming code expects a non-null list.Consider adding a null fallback for consistency:
-(map['<%= attribute.key %>'] as List<dynamic>?)?.map((e) => <%- toPascalCase(attribute.key) %>.values.firstWhere((element) => element.name == e)).toList() +(map['<%= attribute.key %>'] as List<dynamic>?)?.map((e) => <%- toPascalCase(attribute.key) %>.values.firstWhere((element) => element.name == e)).toList() ?? []
155-155: Missing null fallback for relationship arrays.Similar to the enum array issue, relationship arrays don't have a null fallback when the map value is null, which could cause inconsistency with other array types.
Consider adding a null fallback for consistency:
-(map['<%= attribute.key %>'] as List<dynamic>?)?.map((e) => <%- toPascalCase(collections.find(c => c.$id === attribute.relatedCollection).name) %>.fromMap(e)).toList() +(map['<%= attribute.key %>'] as List<dynamic>?)?.map((e) => <%- toPascalCase(collections.find(c => c.$id === attribute.relatedCollection).name) %>.fromMap(e)).toList() ?? []
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
templates/cli/lib/type-generation/languages/dart.js.twig(1 hunks)
🔇 Additional comments (5)
templates/cli/lib/type-generation/languages/dart.js.twig (5)
86-93: LGTM! Well-structured attribute ordering implementation.The implementation properly handles required-first ordering when strict mode is enabled, while preserving original order otherwise. The use of
__attrsas a single source of truth throughout the template is a clean architectural decision.
88-102: LGTM! Effective deduplication of relationship imports.The use of a
Setto track seen imports efficiently prevents duplicate imports for related models. The logic correctly identifies and imports only unique relationship types.
168-181: LGTM! Clean toMap implementation.The toMap method correctly handles all attribute types with proper null-safe navigation. The switch to single quotes for map keys is consistent with Dart conventions.
115-122: LGTM! Consistent field and constructor generation.The field declarations and constructor parameters correctly use
__attrsas the source, maintaining consistency with the ordering logic and properly marking required fields.
138-139: String conversion may cause runtime errors for null values.The
.toString()call on Line 139 will throw ifmap['<%= attribute.key %>']is null for required string fields. This could cause runtime failures during deserialization.Apply this diff to handle null values safely:
-map['<%= attribute.key %>']<% if (!attribute.required) { %>?<% } %>.toString() +<% if (attribute.required) { %>map['<%= attribute.key %>'].toString()<% } else { %>map['<%= attribute.key %>']?.toString()<% } %>Likely an incorrect or invalid review comment.
|
@ChiragAgg5k hello |
| <% } else if (attribute.type === 'float') { -%> | ||
| List<int>.from(map['<%= attribute.key %>'] ?? [])<% } else { -%> | ||
| map['<%= attribute.key %>']<% } -%> | ||
| <% } else if (attribute.type === 'double') { -%> |
There was a problem hiding this comment.
can we use the Attribute.FLOAT typing from attribute.js file?
i know the mismatch there is confusing but lets keep the confusion in one file only 😅
There was a problem hiding this comment.
yes please, also in other places applicable
There was a problem hiding this comment.
in C#, we'll also have to do
| if (a.required === b.required) return 0; | ||
| return a.required ? -1 : 1; | ||
| }); -%> | ||
| <% const __attrs = strict ? sortedAttributes : collection.attributes; -%> |
There was a problem hiding this comment.
a question, we have strict flag only for breaking changes. i.e if you enable strict, the casing might change, leading to mismatch between what the API defines and what type defines
is a different strict ordering really needed here?
There was a problem hiding this comment.
since strict explicitly enables breaking changes, it's best to apply the language's conventions end-to-end — not just casing.
There was a problem hiding this comment.
I also customized the generator for myself, a class with constant field values. It's convenient when you need to update 2-5 fields without creating a model. An idea to think about.
There was a problem hiding this comment.
@Fellmonkey soo ordering by language conversions here is breaking? if not can we follow language conventions by default?
|
@Fellmonkey left some very small comments |
What does this PR do?
Test Plan
Before:
After:
After with -s:
Before (duplicate import):
After: (duplicate import)
Related PRs and Issues
(If this PR is related to any other PR or resolves any issue or related to any issue link all related PR and issues here.)
Have you read the Contributing Guidelines on issues?
(Write your answer here.)
Summary by CodeRabbit