Skip to content
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

Generate actuall enums #931

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

fischerscode
Copy link

This PR changes the enum generation to generate actual enums.

I think sooner or later this is a necessary step to take full advantage of dart`s features.

#862 Also motivates this change.

I tried to keep everything as compatible as possible. I think my implementation should be fully compatible. (Except of sup-typing generated enums. But I think nobody has done this.)

I've grouped my changes into four commits to simplify reviewing this PR.

Close #862

@@ -111,7 +111,7 @@ update-pregenerated: $(PLUGIN_PATH) $(PREGENERATED_SRCS)
protos: $(PLUGIN_PATH) $(TEST_PROTO_LIBS)

run-tests: protos
dart test
dart test && dart -Dprotobuf.omit_enum_names=true test
Copy link
Author

Choose a reason for hiding this comment

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

There are tests that require testing both options.

Copy link
Author

Choose a reason for hiding this comment

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

Turns out this has to be dart -Dprotobuf.omit_enum_names=true test --use-data-isolate-strategy (changed it in 2b145be)
See dart-lang/test#1794 (comment)

Unfortunately this is much slower. It might be best to change the tests that depend on this.

Copy link
Author

Choose a reason for hiding this comment

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

Update: I've introduced the test tag tests_omit_enum_names. (d660015)

' represented.');
out.println('@$coreImportPrefix.override');
out.println("$coreImportPrefix.String toString() => name == '' ? "
'value.toString() : name;');
Copy link
Author

Choose a reason for hiding this comment

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

I had to generate a "custom" toString method for each enum, since the default enum toString takes priority over the one from ProtobufEnum.

This method is designed to be 100% compatible, but it might be better to return Enum.name (super.toString()) instead of name.

@@ -644,7 +644,6 @@ class FileGenerator extends ProtobufContainer {
// Generated code. Do not modify.
// source: ${descriptor.name}
//
// @dart = 2.12
Copy link
Author

Choose a reason for hiding this comment

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

I've removed all of them, since they would mess up the enum files and I don't think they serve any purpose.

@@ -171,7 +172,7 @@ class Imports extends $pb.GeneratedMessage {
/// so the generated code may contain errors. Therefore, running dartanalyzer
/// on the generated file is a good idea.
@$pb.TagNumber(1)
$core.List<DartMixin> get mixins => $_getList(0);
$pb.PbList<DartMixin> get mixins => $_getList(0);
Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure why this change. I assume the files have not been regenerated in the past.

fischerscode added a commit to fischerscode/protobuf.dart that referenced this pull request May 11, 2024
commit 382993b
Author: fischerscode <github@maaeps.de>
Date:   Sat May 11 03:49:05 2024 +0200

    tests

commit 2689dd1
Author: fischerscode <github@maaeps.de>
Date:   Sat May 11 03:48:53 2024 +0200

    also test protobuf.omit_enum_names=true

commit 96d7cad
Author: fischerscode <github@maaeps.de>
Date:   Sat May 11 03:48:38 2024 +0200

    regenerate

commit 64850a2
Author: fischerscode <github@maaeps.de>
Date:   Sat May 11 03:47:46 2024 +0200

    generator
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider generating actual enums
1 participant