[pigeon] Creates new Generator classes for each language #2996
Conversation
|
@gaaclarke I'd love to get another review on the latest commits. Specifically the way the new options are handled. |
I think that looks good. Originally I was concerned that people might mistakenly open file handles to those paths and fiddle around with them instead of using the provided sink. What we found was those file paths were sometimes necessary for generating code. We could probably clean it up with having specific options for the different generators (ie CppHeaderOptions, CppSourceOptions). That's probably not worth the effort though. |
packages/pigeon/lib/pigeon_lib.dart
Outdated
| /// generate. If it returns `null`, the [Generator] will be skipped. | ||
| /// An adapter that will call a generator to write code to a sink | ||
| /// based on the contents of [PigeonOptions]. | ||
| abstract class Adapter { |
There was a problem hiding this comment.
GeneratorAdapter would be a better name; "Adapter" could be adapting anything.
Also, can this be private?
Also, this is a breaking change, so needs to be versioned as such. Everything in Pigeon is directly in lib, so everything that doesn't start with _ is considered public.
There was a problem hiding this comment.
This is currently public for testing purposes. I'd be open to making adjustments to change this though. @gaaclarke mentioned adding the @public_for_testing tag on these classes.
There was a problem hiding this comment.
LGTM with some small nits, but definitely wait for @gaaclarke to review as well.
| } | ||
|
|
||
| /// Generates Dart files for testing with specified [DartOptions] | ||
| void generateTest(DartOptions languageOptions, Root root, StringSink sink) { |
There was a problem hiding this comment.
Optional: Why not make this a file type as well so the structure is the same?
There was a problem hiding this comment.
I was considering that, I just didn't think about it when I wrote the code. I'll change it.
There was a problem hiding this comment.
The issue with this, upon thinking about it more, is that it would generate the test files when generating non test dart code as well. I don't know if that is super important to avoid, but it seems unnecessary.
There was a problem hiding this comment.
We might be able to avoid that by making the set of types option-driven, but let's not worry about it now and just leave it as you have it; we can always revisit later and it's a minor thing.
| abstract class Generator<T> { | ||
| /// Generates files for specified language with specified [languageOptions] | ||
| void generate( | ||
| T languageOptions, Root root, StringSink sink, FileType fileType); |
There was a problem hiding this comment.
Thanks for making the adjustment. I think having a Generator interface and maintaining the Adapter keeps the generator files simple. Everything looks good but one thing. I think I missed some discussion on FileType. I don't think you've really gained anything by its inclusion. I think it's a design mistake for the following reasons:
- Implementing an abstract function and asserting that one parameter is exactly one value means the abstraction doesn't match the implementations well.
- The
FileTypealready has 2 omissions: The AST Generator and the Dart Test generator whose output is not a "source" or a "header" - Considering input to some generators being "header" doesn't make sense in many of the generators.
- The design isn't modular in that new types of output has to be added to a finite list of outputs and all Generators would have to consider what it means to them.
What do we think we gained by adding them? Now there is a grouping of similar generators: CppHeader and CppSource. That seems like it complicates the design with little benefit. What do you think?
There was a problem hiding this comment.
Thanks for the feedback.
Would the inclusion of a default option solve some of these issues?
This allows for generators that don't have multiple file types to ignore the fileType parameter without mislabeling them. It would also avoid the need to add more options to the FileType enum in the future (unless there is some other multi file system that isn't covered by source and header).
Is there a better way to handle combining the generator classes without the use of a parameter similar to this implementation?
There was a problem hiding this comment.
The Generator abstraction is already powerful enough to capture the idea of a group.
or
I still don't think that the group is buying you much in terms of making the codebase easier to understand or work with though.
There was a problem hiding this comment.
What do we think we gained by adding them? Now there is a grouping of similar generators: CppHeader and CppSource.
The issue is that they aren't "similar" they are two fundamentally linked things that can't meaningfully operate on their own. To be useful, each one must be paired with a call to the other with identical inputs. And changes to the header generator almost always require exactly corresponding changes to the source generator or everything breaks.
That's why I would like to see combined classes for all of the linked groups; the goal here is to move all of the generator logic into the subclasses, and inherently codependent logic should be in the same class.
(Having two parallel classes, each with half the logic, feels like when someone has two Ivar arrays where the object indexes must match, instead of using a map or an array of structures. Technically it works, but it's really fragile and harder to understand.)
There was a problem hiding this comment.
Working on a design doc for this to allow for more effective communication about our goals here. https://docs.google.com/document/d/1ABr5yZM_sOZTd66bIVpsCJ7_koKmH0tZuLH4ZT1cs8c/edit?usp=sharing
|
closed for #3022 |
Hopefully less controversial version of #2985.
Creates new
Generatorsuperclass and new language specific subclasses.Also renames former
Generatorclasses toGeneratorAdapterand updates tests accordingly.Initial set up for flutter/flutter#117416