-
Notifications
You must be signed in to change notification settings - Fork 184
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
base: master
Are you sure you want to change the base?
Conversation
protoc_plugin/Makefile
Outdated
@@ -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 |
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.
There are tests that require testing both options.
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.
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.
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.
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;'); |
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.
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 |
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.
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); |
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.
I'm not sure why this change. I assume the files have not been regenerated in the past.
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
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