Add support for manually defining fields #1156
Conversation
WalkthroughThis update introduces significant modifications to the Freezed package. The changelog now reflects the removal of deprecated Changes
Sequence Diagram(s)sequenceDiagram
participant D as Declaration
participant FG as FreezedGenerator
participant C as Class Model
D->>FG: parseDeclaration(declaration, annotation)
FG-->>D: Return parsed Class (using pattern matching)
FG->>C: Construct updated Class structure
sequenceDiagram
participant U as User Code
participant F as Freezed Factory (Subclass)
participant B as Base Constructor
U->>F: Invoke Subclass factory constructor
F->>B: Call Base.named() with parameters
B-->>F: Base instance created
F-->>U: Return Subclass instance with copyWith functionality
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (6)
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 (16)
packages/freezed/lib/src/templates/from_json_template.dart (5)
9-11: Use a more descriptive parameter name for maintainability.While it’s clear that
clazzreferences theClassmodel, consider renaming this field to something more descriptive, liketargetClassormodelClass, to improve readability.
20-20: Return an indicative comment for single-constructor case.Returning an empty string can be ambiguous when reading generated code. Consider returning a short comment or explanation instead, so generated artifacts remain more traceable.
23-23: Ensure clarity in error messaging.The updated error message references
clazz.name, but it might help to specify which specific constructors are colliding. Consider appending details about both the class name and the redirected name to aid debugging.
36-36: Maintain code symmetry.The
elsebranch sets up a union switch. Consider placing a comment here mirroring the single-constructor explanation to ensure future readers know how branches differ.
51-57: Refine fallback exception message.Relying on
CheckedFromJsonExceptionis great, but consider clarifying the union key in the error message, especially if multiple nested keys exist. For instance, specify'Invalid union type "\${json['${clazz.options.annotation.unionKey}']}" for class ${clazz.name}'.packages/freezed/lib/src/freezed_generator.dart (2)
65-66: Add test coverage for single constructor scenario.You are explicitly handling a single constructor scenario with
constructorsNeedsGeneration case [final ctor]. Make sure to unit-test that logic, including custom parameter decorators.
69-70: Refine property filtering.Here
result.readableProperties.addAll(...)runs awhere(...)check referencing synthetic parameters. Cross-check if you need to handle additional property conditions (e.g., skip certain annotation types).packages/freezed/lib/src/templates/concrete_template.dart (8)
62-64: Collect parameters for@JsonSerializable.Ensuring no duplicates (e.g.,
createToJsonrepeated twice) can help avoid potential code generation conflicts. All parameters look correctly appended.
83-96: Mapping parameters withmapParameters2.The new approach is more flexible but keep an eye out for corner cases, such as optional positional parameters overshadowing named parameters. Document how collisions are addressed.
114-122: Conditional super parameter insertion.Using the
isNamed && superCall.positional.contains(p.name)check is a solid approach. Consider the edge case of multiple overlapping parameters if the constructor uses different styles.
136-141: Only wrap synthetic collection fields.Filtering
e.isSyntheticfor unmodifiable collections helps keep user-specified fields intact. Provide a comment clarifying that user-coded fields remain unwrapped.
294-296: Valid fromJson parameter composition.This logic elegantly composes the JSON parsing arguments. Test for edge cases, such as generics with default type parameters or high-level constraints.
Would you like me to propose extended test scenarios?
327-338: Conditional Diagnostics usage.When
globalData.hasDiagnosticsis true, the debug fill properties add helpful metadata. This is good for enhanced debugging; watch for performance overhead in large classes.
350-350: String interpolation intoString.
for (final p in constructor.properties.map((e) => e.value)) ...ensures a descriptive toString. If properties are large objects, consider truncation or summary output for performance.
369-369: Equality checks for special naming.If a property is named
other, substitutingthis.is a neat trick. Just confirm no conflict arises with a property literally calledthis.packages/freezed/test/integration/multiple_constructors.dart (1)
32-33: Fix the incomplete comment.The comment appears to be incomplete and should be removed or completed.
- // Check that no @override is nu
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
packages/freezed/CHANGELOG.md(1 hunks)packages/freezed/lib/src/freezed_generator.dart(6 hunks)packages/freezed/lib/src/models.dart(14 hunks)packages/freezed/lib/src/templates/abstract_template.dart(2 hunks)packages/freezed/lib/src/templates/concrete_template.dart(10 hunks)packages/freezed/lib/src/templates/copy_with.dart(3 hunks)packages/freezed/lib/src/templates/from_json_template.dart(2 hunks)packages/freezed/lib/src/templates/parameter_template.dart(4 hunks)packages/freezed/test/deep_copy_test.dart(1 hunks)packages/freezed/test/integration/deep_copy.dart(1 hunks)packages/freezed/test/integration/multiple_constructors.dart(1 hunks)packages/freezed/test/integration/single_class_constructor.dart(1 hunks)packages/freezed/test/single_class_constructor_test.dart(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: freezed (packages/freezed_lint, master, get)
- GitHub Check: freezed (packages/freezed_annotation, master, get)
- GitHub Check: freezed (packages/freezed, master, get)
🔇 Additional comments (58)
packages/freezed/lib/src/templates/from_json_template.dart (6)
15-16: Validate conflict detection logic.The updated logic uses
clazz.constructors.where(...)to detect naming conflicts, which is appropriate for multi-constructor classes. Verify that partial matches (e.g., if the constructor name is a substring of the class name) don’t cause accidental conflicts.
30-34: Good modernization of single-constructor handling.This direct approach to single-constructor fromJson is concise and reads well. Confirm coverage in unit tests, especially where
genericArgumentFactoriesmight be involved.
59-59: Check fallback uniqueness.Using
singleWhereOrNullfor the fallback constructor logic is fine if you expect at most one fallback constructor. If multiple fallback constructors could appear, consider raising a compile-time or generation-time error.
61-64: Confirm fallback constructor usage.The fallback constructor logic is clear, but be sure to confirm in tests that it doesn't overshadow non-fallback constructors. In some edge scenarios, multiple matches could arise if user code configures multiple “fallback” constructors.
68-68: Clear union key switch structure.Switching on
${clazz.options.annotation.unionKey}is a solid approach for multi-constructor classes. This is consistent with the library’s design.
76-79: Suffix naming consistency.The function name
_\$${clazz.name.public}FromJsonhelps maintain the naming convention across generated artifacts. Ensure other parts of your codebase using a different prefix/suffix are refactored for consistency.packages/freezed/lib/src/freezed_generator.dart (6)
38-39: Consistent naming for_parseDeclarationparameters.Changing from
LibraryDatatoLibraryemphasizes the updated data structure. Ensure references across the code use the new, more descriptive type name consistently.
57-57: Switching toClass.fromis a cohesive approach.Using
Class.from(declaration, configs, ...)is consistent with the consolidation of data types. Just verify you are covering all oldData.fromusage in the rest of the codebase.
60-61: Check common parameter logic for multi-constructor classes.
_commonParametersBetweenAllConstructors(Class data)extracts shared parameters. Confirm it handles advanced cases (e.g., optional vs. required parameters, or partial overlaps).
218-219: Well-defined library instantiation.
final library = Library.from(units);is aligned with the new approach. Confirm that newly introduced methods onLibraryhandle all metadata and constructs from multiple compilation units.
[verified]
223-227: Batch parsing of user-defined classes is concise.
_parseDeclarationis invoked in a loop over annotations, ensuring each decorated class is processed. This design is maintainable and easy to extend if new annotation-based logic emerges.
229-230: Parallel generation for each class.
_generateForDatais iterated over all parsed user-defined classes. Make sure to handle the case where no constructors exist (e.g., abstract classes).packages/freezed/lib/src/templates/concrete_template.dart (11)
25-26: Data model alignment looks good.Switching to
final Class data;andfinal Library globalData;maintains consistency with the new codebase structure. This is a preferable approach for clarity.
32-33: Union key property detection.Checking
constructor.properties.every((e) => e.value.name != data.options.annotation.unionKey)ensures no collisions. Confirm you have coverage in tests for union keys that match property names unintentionally.
38-38: Delegated_jsonSerializable()builder is a neat refactor.Moving the annotation creation into a private method
_jsonSerializablefosters separation of concerns. This helps keep thetoStringmethod minimal and more maintainable.
67-68: Conditionally enablegenericArgumentFactories.Checking for param count under
data.genericsParameterTemplate.typeParameters.isNotEmptyis good. Ensure your condition handles generic constraints or where clauses.
109-111: Inline super call logic.The
data.superCall!usage is straightforward. Ensure that super calls are optional ifdata.superCallcan be null or incomplete in user code.
124-127: Fallback to local parameter if not matched.You handle the remainder with a
Parameter.fromParameter(p). This ensures you don’t lose parameters not insuperCall. Good coverage for typical usage.
130-130: Parameter consolidation.Combining named and positional parameters for the trailing constructor expression is clean. Watch for clarity if more logic is inserted here.
132-132: Post-constructor initialization block.Storing unmodifiable checks or assertion expansions in
trailingStringsis neat. This pattern ensures the constructor body remains compact.
142-205: Union key injection and class heritage.The logic to inject
$typeif_hasUnionKeyPropertyis false is well-structured. In the_concreteSuperpart, you addextends,with, andimplements. All looks consistent with multi-constructor class patterns.
277-281: Applying@overrideon common properties.Applying
@overridefor combined or inherited properties ensures correct overriding behavior. Just confirm it’s not used on properties that don’t exist in the parent class.
308-310: Reliable toJson argument composition.Similar to the fromJson logic, ensure the
toJsonArgsaccounts for complex generic constraints.packages/freezed/lib/src/models.dart (18)
159-159: Use of newpropertiesfield name looks clear.
RenamingimpliedPropertiestopropertiesclarifies intent and is consistent with the new approach. No immediate issues.
173-210: Validate or test positional parameter checks.
The_assertValidNormalConstructorUsagemethod introduces stricter validation for a private'_'constructor alongside factory constructors. This enforces consistent parameter usage across constructors. Ensure all constructors are thoroughly tested, especially with positional parameters.Would you like a script to scan for multiple constructors with mismatched parameters for additional verification?
242-245: Good approach for capturing the manual constructor.
Storing the_constructor asmanualConstructoris a clear way to differentiate manual vs. factory usage. No concerns here.
249-249: Ensure forced validation on normal constructors is intended.
Calling_assertValidNormalConstructorUsagefor non-factory constructors ensures alignment with the internal rules. This might be strict, so confirm you want to enforce the single private named constructor for all normal usage.
253-253: Safe extraction ofredirectedName.
Retrieves the identifier of the redirected constructor. Straightforward, no issues noted.
257-257: Double-check repeated usage of_assertValidNormalConstructorUsage.
Invoking_assertValidNormalConstructorUsageagain for non-redirecting constructors might cause duplicate errors if the same issues appear in multiple places. Consider ensuring errors won't be thrown twice.
266-270: Excluded properties set captures manually handled fields.
Filtering out parameters accounted for by the manual constructor is a neat approach. Check that no unexpected property is excluded.
271-281:allPropertiesitem bundling is well-structured.
Storing(isSynthetic, value)helps track manual vs. generated fields. Looks good for code generation.
292-292: AssigningallPropertiestopropertiesis consistent.
Straightforward property initialization, aligned with earlier changes. No issues.
373-380: Utility methods for callback naming and synthetic checks.
The newcallbackNameandisSyntheticmethods extend functionality nicely. EnsurefirstOrNullusage never yields null references when calling.isSynthetic.
389-391:ImplementsAnnotation.parseAllclarity.
This loop properly collects annotations. No immediate concerns, but confirm that ignoring unresolved annotations is deliberate.
398-398: Safe cast toInterfaceType.
Double-check that the cast(meta.type! as InterfaceType)is always valid. Consider additional checks if the annotation might not be anInterfaceType.
482-482: IntroducingsuperCallas a required constructor param.
RequiringsuperCallensures that any subclass leveraging_is explicit. Should be fine.
490-490: NullablesuperCallfield.
Allowing null is sensible—some classes won’t have a private constructor.
492-499: Factory method forClass.fromwithprivateCtordetection.
Collecting the private constructor early clarifies whether manual fields or extends-like behavior is allowed. This code is consistent with the rest of the logic.
502-503: Validating fields with_assertValidFieldUsage.
Adds an extra check if the class is effectively extending. Good approach to enforce immutability constraints.
511-524:superCallbuilding for private constructor is well-structured.
ConstructingSuperInvocationwith positional/named parameters ensures consistent code generation. This should handle advanced inheritance or_usage.
586-587:Library.frommethod improves discoverability.
Having a single entry point to buildLibraryfrom a list ofCompilationUnits is neat. No issues noted.packages/freezed/lib/src/templates/abstract_template.dart (2)
15-15: Changed field type fromDatatoClass.
The renaming aligns with the new model abstraction. Ensure all references usingAbstract.datahave been updated accordingly elsewhere.
44-44: Usingdata.options.genericArgumentFactoriesoverdata.genericArgumentFactories.
Flags thatgenericArgumentFactoriesnow resides inoptions. This is consistent with the refactor. Just confirm external references are updated.packages/freezed/test/integration/deep_copy.dart (2)
5-8: New classDeepWithManualField.
Having a single field of typeManualFieldextends deep-copy testing. Ensure coverage via test that modifies the manual field.
10-16: Manually declared_constructor forManualField.
This is a compelling usage for partial customization while still supportingcopyWith. Looks correct. Confirm no conflicts with code generation for private constructors.packages/freezed/test/integration/multiple_constructors.dart (2)
5-15: LGTM! Well-structured class with flexible parameter ordering.The
ManualPositionInAnyOrderclass effectively demonstrates how to handle the same parameters in different orders using multiple constructors.
17-24: LGTM! Good use of optional parameters with default values.The
ManualNamedOptionalPropertyclass shows a clean implementation of optional parameters with default values.packages/freezed/lib/src/templates/copy_with.dart (1)
38-38: LGTM! Improved encapsulation with private getter.The renaming of
canAccessRawCollectionto_canAccessRawCollectionbetter reflects its internal usage.Also applies to: 42-42
packages/freezed/test/integration/single_class_constructor.dart (2)
10-14: LGTM! Well-structured base class.The
Baseclass provides a clean foundation for inheritance with clear parameter requirements.
16-20: LGTM! Good use of inheritance with Freezed.The
Subclassdemonstrates proper inheritance fromBasewhile utilizing Freezed's features and providing sensible defaults.packages/freezed/lib/src/templates/parameter_template.dart (3)
144-187: LGTM! Enhanced parameter mapping with improved type safety.The
mapParameters2method provides a more robust way to map parameters with additional context about their position and requirements.
326-374: LGTM! Well-implemented parameter class for super parameters.The
SuperParameterclass provides a clean implementation for handling super parameters with proper formatting and initialization.
377-391: LGTM! Improved constructor initialization using super.The updated constructors in
LocalParameterandCallbackParameteruse thesuperkeyword for cleaner initialization.Also applies to: 427-442
packages/freezed/test/single_class_constructor_test.dart (1)
187-207: LGTM! Well-structured test case.The test case thoroughly verifies the essential functionality of the
Subclassclass, including object equality, hash code consistency,copyWithbehavior, and string representation.packages/freezed/test/deep_copy_test.dart (1)
453-460: LGTM! Focused test case.The test case effectively verifies the
copyWithfunctionality for manual fields, ensuring that the value is correctly updated through thecopyWith.manualchain.packages/freezed/CHANGELOG.md (3)
3-4: LGTM! Clear documentation of breaking changes.The breaking changes are well-documented and clearly marked, helping users understand the impact of upgrading to version 3.0.0.
5-19: LGTM! Clear inheritance example.The inheritance example effectively demonstrates how to extend a base class and pass super values through the
._constructor.
20-31: LGTM! Clear dynamic default values example.The dynamic default values example clearly shows how to use the
._constructor to set default values conditionally.
| final correspondingProperty = constructor.properties | ||
| .where((element) => element.value.name == p.name) | ||
| .first; | ||
| if (correspondingProperty.isSynthetic) { | ||
| return ( |
There was a problem hiding this comment.
Handle synthetic property detection carefully.
.where((element) => element.value.name == p.name).first might throw if no matching property is found. Consider using .firstWhereOrNull(...) or verifying presence.
Proposed fix:
- final correspondingProperty = constructor.properties
- .where((element) => element.value.name == p.name)
- .first;
+ final correspondingProperty = constructor.properties
+ .firstWhere(
+ (element) => element.value.name == p.name,
+ orElse: () => throw InvalidGenerationSourceError(
+ 'No matching property found for parameter "$p"'),
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| final correspondingProperty = constructor.properties | |
| .where((element) => element.value.name == p.name) | |
| .first; | |
| if (correspondingProperty.isSynthetic) { | |
| return ( | |
| final correspondingProperty = constructor.properties | |
| .firstWhere( | |
| (element) => element.value.name == p.name, | |
| orElse: () => throw InvalidGenerationSourceError( | |
| 'No matching property found for parameter "$p"'), | |
| ); | |
| if (correspondingProperty.isSynthetic) { | |
| return ( |
First steps toward inheritance & dynamic default values.
This adds support for defining values in the
._()constructor:Summary by CodeRabbit
New Features
Refactor
Tests