Skip to content

[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

Merged
merged 36 commits into from
Jan 6, 2023

Conversation

tarrinneal
Copy link
Contributor

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

/// Generates Cpp files with specified [CppOptions]
@override
void generate(CppOptions languageOptions, Root root, StringSink sink,
{required FileType fileType}) {
Copy link
Member

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.

Copy link
Contributor

@stuartmorgan-g stuartmorgan-g Jan 5, 2023

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).

@tarrinneal tarrinneal requested a review from gaaclarke January 5, 2023 22:47
Copy link
Member

@gaaclarke gaaclarke left a 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.

Copy link
Contributor

@stuartmorgan-g stuartmorgan-g left a 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.

@tarrinneal tarrinneal added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 6, 2023
@tarrinneal tarrinneal merged commit 25454e6 into flutter:main Jan 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: pigeon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants