Skip to content
This repository was archived by the owner on Jul 27, 2022. It is now read-only.

Makes setup of transformer based serialization more simple.#9

Open
nicolasgarnier wants to merge 38 commits intogoogle:masterfrom
nicolasgarnier:simple-setup
Open

Makes setup of transformer based serialization more simple.#9
nicolasgarnier wants to merge 38 commits intogoogle:masterfrom
nicolasgarnier:simple-setup

Conversation

@nicolasgarnier
Copy link
Copy Markdown

The goal of this PR is to simplify the setup of transformer based serialization while keeping it flexible and also make the Editor happy (no more importing of not-yet generated files). This includes the following changes:

  • Generate one file containing all generated serialization rules
  • Automatically replace the ...serialization.dart import with the generated Serialization rules file. It overrides the constructors so that all generated rules are automatically added.
  • Generate rules for Classes with the @serializable() annotation.
  • Fixed bug where "part of" files where not imported properly (because we need to import the main library file).
  • Allow to process classes of files that are in imported packages (couldn't do that with the $include: ["lib/stuff.dart", "lib/more_stuff.dart" ...] in the old transformer).

With these changes developers can just do:

import 'package:serialization/serialization.dart';
...
var serialization = new Serialization();

Person person = new Person();
...
var personJson = serialization.write(person);

And the pubspec.yaml declaration looks like this:

transformers:
- serialization :
  useAnnotation: true
  files:
  - bin/stuff.dart
  - lib/more_stuff.dart
  - package:package_name/3rd_party_stuff.dart #for imported packages

…ly copied by the transformer and rules are added to it.
…ing changes:

 - Generate one file containing all generated serialization rules (actually it can be 2 files, one at the root of the project and one at the root of the packages/... folder in the case there was a lib/ folder. Because the lib/ only has access to potentially a subset of the Types and generated rules.)
 - Automatically replace the ...serialization.dart import with the generated Serialization rules file. It overrides the constructors so that all generated rules are automatically added.
 - Generate rules for Classes with the @serializable() annotation.
 - Fixed bug where "part of" files where not imported properly (because we need to import the main library file).
 - Allow to process classes of files that are in imported packages (couldn't do that with the $include: ["lib/stuff.dart", "lib/more_stuff.dart" ...] in the old transformer).
@nicolasgarnier
Copy link
Copy Markdown
Author

Not sure this should be version 1.0.0 though. Should I switch to v 0.2.0 ? (does that clearly indicate that there are breaking changes?)

@alan-knight
Copy link
Copy Markdown
Contributor

I definitely wouldn't make it a 1.0.0. The convention we've been using for pre-1.0 packages would be that 0.2.0 has potentially breaking changes. Except 0.2.0 is already taken. It would be 0.11.0.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why the hide? I don't think that file exports Serialization.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Not sure why I did that :) Removed :)

@nicolasgarnier
Copy link
Copy Markdown
Author

All tests should be passing in Travis now :)

…erated serialization rules since the import will be handled by the transformer.
…. For this we import all models with as "as" import statement and the generated rule class name also has the same prefix.
@nicolasgarnier
Copy link
Copy Markdown
Author

Just made a bunch of changes (no more should come unexpectedly). The generator now handles imports with conflicting class names by adding an "as" import statement to all imports in the generated rules file.

Comment thread CHANGELOG.md
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The way you specify the files to the transformer is also changed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why change this from using inheritance to delegating? It seems like the user has to write an extra 7 lines of boilerplate.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

There are a few reasons I did this:

  • The editors show an error when using ThingWithDateSerializationRule because it's not generated yet.
  • To avoid collision of rules names I need to add a prefix to the generated rule class name (currently using a counter so the current name is: _0ThingWithDateSerializationRule). That's in the case multiple class named DateSerializationRule from different files are imported.
  • It feels that by using ThingWithDateSerializationRule the user is relying too much on implementation details (matching naming) whereas with the above he is now relying on defined interface (CustomRule and Serialization.automaticRules[]) available before transformer run...

Thing is I would like to be able to keep this and have stable naming but now that all rules are in the same file there is a risk of name collision (actually happening in the tests). Not sure how to work around that... Or maybe we can use another prefix for the generated rule class name like: $filename_ThingWithDateSerializationRule ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we also have the ability to run this at development-time, as discussed separately, that seems like it would address the editor error. You could even mix and match them, that is you could run the generation once, so the editor has not just the files but also the rules, but they aren't updating automatically. Then you could have the transformer update them.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Collisions should be rare. I think the best option would be to have a parameter on the annotation. So, e.g. @serializable(ruleName: 'FooRuleWithMyName')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I also think we will want a parameter for annotating a class you don't control, e.g. @serializable(class: DateTime, ruleName: MyDateAndTimeRule). There was a mention in the misc thread today of someone who did not want to modify his model sources in any way, including annotations. I think being able to have the annotations separately is probably adequate for that, but not sure.

Comment thread pubspec.yaml
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do we have both files: and $include: forms?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It seems that one particular transformer can't be ran twice on the same asset. I get an error. It may be a bug in Barback. But basically to be able to run the transformer twice I had to make sure they tun on a completely different set of files via this $include and $exclude. Looks like a bug (or "limitation") of transformers... but I had no choice.

@alan-knight
Copy link
Copy Markdown
Contributor

I'll stop commenting on details. I think, per this, and discussion elsewhere that the thing to do is to disentangle this a bit from transformers. So, roughly

  • be able to generate the code manually, including as an empty file
  • use that empty file as the import rather than replacing the serialization import

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants