Skip to content

Conversation

@timmaffett
Copy link
Contributor

This PR adds a subdirectory augment_test within test that is a copy of the test directory, with the only changes being that the model classes have been edited to use the forthcoming dart augmentation capabilities/experimental feature.

My intention here is not to have this PR merged, but to spur discussion and to discover bugs and limitations in the current dart augmentation experimental implementation.

I am imagining here the code that a possible @Codable() annotation, implemented via a build_runner builder, might automatically produce,

I did discover both crashes and bugs that can be triggered within the analyzer when using the augment keyword. Fortunately, these are only triggered by using augment to create the generic and polymorphism models.
So far I have filed dart-lang/sdk#60040 and dart-lang/sdk#60039

The augmentation code that causes the analyzer crashes has been commented out for now and left as the original examples defined the classes (to leave the analyzer intact/crash free within the VSCode environment). The code has been commented to indicate what causes the crashes (or incorrect analyzer errors) and the temporary (unaugmented) code is commented/highlighted.

(I still need to isolate/create standalone version of the crashes and polymorphism errors, see README.md within the augment_test directory for more info and references to where these problems occur.)

The only modified files within the augment_test directory are

basic/model/person.dart
basic/model/person.codable.dart
csv/model/measures.dart
csv/model/measures.codable.dart
enum/model/color.dart
enum/model/color.codable.dart
generics/model/box.dart                  <<BUG in augmentation/analyzer found here 
generics/model/box.codable.dart    <<BUG in augmentation/analyzer found here
polymorphism/basic/model/pet.dart
polymorphism/basic/model/pet.codable.dart
polymorphism/complex/model/box.dart                     <<BUG in augmentation/analyzer found here
polymorphism/complex/model/box.codable.dart       <<BUG in augmentation/analyzer found here
polymorphism/multi_interface/model/material.dart
polymorphism/multi_interface/model/material.codable.dart

I included all of the other test files in the augment_test directory so that the tests could be used to verify that the augment versions work identically. (Currently they CANNOT be run because the dart compiler has an error preventing any use of the augment keyword from compiling (or even formatting)) dart-lang/sdk#60039

@schultek
Copy link
Owner

schultek commented Feb 4, 2025

This is really cool, I need to take a deeper look when I got time.

late String? name;
late int age;
late bool isActive;
late DateTime? signupDate;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nullable options should not be late, since they may simply not appear at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch @TekExplorer ! In the case of nullable fields within the model the code generator should not generate late modifiers because the runtime check for initialization (forced by using late) would generate an exception when the field had not been initialized explicitly to null.
This block should look like this

    late String id;
    String? name;
    late int age;
    late bool isActive;
    DateTime? signupDate;
    Uri? website;

So name and signupDate auto initialize to null (and could possible be changed to something else during the decode).
@schultek Thoughts ?

@TekExplorer
Copy link

i like the usage of augmentations, but i also think that the implementation as it is is also super cluttered.

i suggest combining the implementation details that arent relevant, or even remove them entirely. stuff like toString, ==, hashcode, can be implied since these are examples.

If you do not want to remove them, at least put them in the same augmentation.

additionally, you switch from field-first to field AND constructor at the inheritance part. i would suggest taking the freezed route and going constructor-first, or just have constructors everywhere. that may be more consistent if you choose to remove the "data class" code

Its just a bit jarring.

@timmaffett
Copy link
Contributor Author

@TekExplorer - The reasons I have multiple augmentation entries in the examples is that I was trying to mimic what might happen if a code generator was creating separate augmentation blocks as a result of separate/disparate annotations.

In the examples I ended up going with
@Codable(equatable:true,toString:true) for the annotation syntax,

but I could also imagine something like

@Codable
@Equatable
@ToString

in which case the annotations would each be generating there own code blocks (and therefor separate augmentation blocks).

With the syntax that I went with it should be possible to combine the different augmentation blocks in to a single block because it is a single annotation ( @Codable(equatable:true,toString:true) ) that is triggering the generation of the code.

I was just trying to exercise the augmentation parsing/compiling to make sure that it would be possible for the blocks to be separate.

We suspect will also want to support separate @Decodable and @Encodable annotations, so it is quite possible that we would have two augmentation blocks based on the two different annotations which are triggering the respective code generation.

@TekExplorer
Copy link

yes, but i dont think its a good idea to confuse the examples with out-of-scope features. the point of this is Codable, not equatable and tostring.

@timmaffett
Copy link
Contributor Author

I understand the point you are making, but my intentions here were not to create simplified examples.
To be clear - this branch is creating augmentation prototypes of the original examples, without changing the functionality of the original examples, with the end goal being the ability to run the two different versions side by side and verify identical output.

Some of the original examples included equitability and toString functionality, so these augmentation examples do as well.

I will comment that I do think it would be worthwhile for the Codable annotations to be able to optionally implement this functionality to it's Codable augmentations.

@TekExplorer
Copy link

I understand the point you are making, but my intentions here were not to create simplified examples.
To be clear - this branch is creating augmentation prototypes of the original examples, without changing the functionality of the original examples, with the end goal being the ability to run the two different versions side by side and verify identical output.

Some of the original examples included equitability and toString functionality, so these augmentation examples do as well.

Okay

I will comment that I do think it would be worthwhile for the Codable annotations to be able to optionally implement this functionality to it's Codable augmentations.

Eh, I don't really agree. Notice that json_serializable doesn't do that, while freezed does.

The difference being, json_serializable, like Codable, is purely about serialization, which may not necessarily be "data class". You can have mutable, serializable classes.

Freezed, however, is focused on data classes, which usually implies serialization (not the other way around) and doesn't even do it itself - it just supports it with json-serializable.

Let Codable focus only on what it's intended to do.

@timmaffett
Copy link
Contributor Author

I will comment that I do think it would be worthwhile for the Codable annotations to be able to optionally implement this functionality to it's Codable augmentations.

Within the example code you can find (in comments) purposed annotations such as
@Codable - denotes creating the codable serialization/deserialization code only
and then some
@Codable(equatable:true,toString:true) - denotes also generating equatable and toString methods.

These arguments would be optional.

I personally would like Codable to generate this repetitive (and laborious and time consuming) code for me, as it continually changes as the models change (just as the serialization code).
Yes - we could rely on other annotation packages to do this, and that would still be possible (just use the @Codable annotation), but it seems like it would be nice for Codable to be able to optionally generate these convenience methods.

I will also restate that as everything in this PR these are just ideas I extrapolated from @schultek 's original RFC code and things such as the annotation syntax were specified as to be clear in the example code and spark discussion (such as this).

ps.
That is also why the equatable and toString methods appear in separate augmentation blocks - because it is possible we would want to use a separate annotation to trigger these (instead of arguments on @Codable) and I also was exercising the augmentations' ability to use the separate blocks (which could even be generated by completely separate packages)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants