-
Notifications
You must be signed in to change notification settings - Fork 3.3k
[pigeon] Creates new Generator classes for each language (v3) #3022
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
/// Generates Cpp files with specified [CppOptions] | ||
@override | ||
void generate(CppOptions languageOptions, Root root, StringSink sink, | ||
{required FileType fileType}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, this looks good to me. I'd just pull fileType
into the CppOptions since it isn't applicable to all Generators.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It applies to two already, and will apply to three when we add Linux, so whatever solution we have shouldn't be specific to C++.
If we put it in options, we won't be able to pass the same immutable options to each generate call, which undermines the idea that all calls to generate need to happen with identical options (so that the generated code matches).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks. I think this is much better than where we were before vacation. Now at every step of the calculation there isn't superfluous input to any of the procedures. No "ignore this parameter since it isn't applicable" and the input to the procedures is limited in scope to just what the generator needs (for the most part). Thanks so much for looking into this Tarrin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small naming issue, but otherwise LGTM.
Slightly modified version of #2996.
Adds NA option to FileType Enum. Uses it as default value in (newly) name parameter - fileType - in relevant Generators.
Creates new Generator superclass and new language specific subclasses.
Also renames former Generator classes to GeneratorAdapter and updates tests accordingly.
Initial set up for flutter/flutter#117416