[pigeon] Adds StructuredGenerator class and subclasses #3037
Conversation
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
LGTM, I have a few nits below. I also think the namespace code could have space in the abstract base class and that encode/decode implementations are empty but that could be addressed later if we want.
Sorry I didn't get a chance to do this today; I started but didn't get very far. Hopefully it wasn't too painful to integrate my change. |
stuartmorgan-g
left a comment
stuartmorgan-g
left a comment
There was a problem hiding this comment.
I have a few small comments, and one larger comment for later consideration, but overall this looks great. Having this coherent structure will be a really big win for maintainability even before we potentially start extracting more shared logic.
I also really, really like having the checked in output files for this kind of PR. Being able to see the (trivial) output changes makes it really easy to have confidence that nothing got accidentally changed in the reorganizing.
packages/pigeon/lib/generator.dart
Outdated
| ) { | ||
| final Indent indent = Indent(sink); | ||
|
|
||
| writeFilePrologue(generatorOptions, root, sink, indent); |
There was a problem hiding this comment.
(At some point I'd like to revisit whether these other things that are passed to literally every method could be instance variables instead, but since they can't be constructor arguments due to the way generate is set up let's have that discussion later so it doesn't block this PR.)
There was a problem hiding this comment.
I would prefer to avoid the repeating parameters also
packages/pigeon/platform_tests/test_plugin/windows/pigeon/core_tests.gen.h
Outdated
Show resolved
Hide resolved
packages/pigeon/platform_tests/test_plugin/windows/pigeon/core_tests.gen.h
Outdated
Show resolved
Hide resolved
tarrinneal
added
the
autosubmit
3 participants
Adds StructuredGenerator class and subclasses
fixes flutter/flutter#117416