-
Notifications
You must be signed in to change notification settings - Fork 195
Follow Protobuf Enum style through a new cli option "protobuf-enum-style" #1045
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
base: master
Are you sure you want to change the base?
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
…efix from enum value and UpperSnakeCase to CamelCase
Currently `deepCopy` and `clone` create an empty message, and then merge the cloned message into the empty message. This is slow as merge needs to validate each element added to the message. In this PR we instead directly copy the field set, and deeply copy map, list, and message fields. deepCopy benchmark results: | | Before | After | Diff | |------------------|---------|---------|--------| | AOT | 238 us | 151 us | -36% | | dart2js -O4 | 242 us | 140 us | -42% | | dart2wasm -O2 | 230 us | 123 us | -46% | | JIT | 189 us | 104 us | -44% | This CL is tested internally in cl/794080754. Fixes google#60.
This fixes handling of unknown enum values in repeated and map fields and in top-level fields. Current behavior: - Unknown enums in map and repeated fields throw an exception. - Unknown enums in top-level fields reset the field value to the default one. With this PR we just "ignore" the values: - Unknown enums in map and repeated fields are skipped. - Unknown enums in top-level fields do not reset the field's value to the default. Fixes google#849.
Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…d code (google#1040) In `binary_decode_packed` benchmarks we used "sink" variables to write the decoded values, and printed these sinks at the end to make sure they're not dropped. Do the same in the rest of the benchmarks to make sure no code is eliminated by a sufficiently smart compiler or VM. Just as a sanity check I compared the numbers before and after this change, when compiled to Wasm with `-O2`. The numbers do not change in a consistent (reproducible) and significant way. It's still good to be safe and use the benchmark results to avoid dropping code.
In a declaration like @js('JSON') extension type _JSON._(JSObject _) implements JSObject { @js('JSON.stringify') external static JSString _stringify(JSObject value); } When I call `_JSON._stringify(...)`, dart2js calls `JSON.stringify`, but dart2wasm calls `JSON.JSON.stringify`, which is wrong. Reading [1] it looks like the annotation `JSON.stringify` should just be `stringify`. If I do that the code works with both dart2wasm and dart2js. (I've reported dart2wasm's handling of the annotations above and we'll work on a fix for it on dart2wasm.) This change is not tested on the CI because when compiling with dart2wasm we use the JS decoder library used by the VM, which doesn't use JS interop. I tested this manually with the patch: diff --git a/protobuf/lib/src/protobuf/json/json.dart b/protobuf/lib/src/protobuf/json/json.dart index 05d1ac1..70b6a7c 100644 --- a/protobuf/lib/src/protobuf/json/json.dart +++ b/protobuf/lib/src/protobuf/json/json.dart @@ -13,7 +13,7 @@ import '../utils.dart'; // Use json_vm.dart with VM and dart2wasm, json_web.dart with dart2js. // json_web.dart uses JS interop for parsing, and JS interop is too slow on // Wasm. VM's patch performs better in Wasm. -export 'json_vm.dart' if (dart.library.html) 'json_web.dart'; +export 'json_vm.dart' if (dart.library.js_interop) 'json_web.dart'; Map<String, dynamic> writeToJsonMap(FieldSet fs) { dynamic convertToMap(dynamic fieldValue, int fieldType) { [1]: https://dart.dev/interop/js-interop/usage#js
Keep the symbols to get useful perf outputs.
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 added comments to help the reviewer understand what the changes are and why this does not bring any breaking change.
// Generate Dart name for the enum value | ||
String dartName; | ||
final isProtobufEnumStyle = | ||
parent.fileGen?.options.protobufEnumStyle ?? false; | ||
if (isProtobufEnumStyle) { | ||
// Strip enum prefix from protobuf-style enum values | ||
final strippedName = stripEnumPrefix(descriptor.name, value.name); | ||
dartName = avoidInitialUnderscore(strippedName); | ||
} else { | ||
// Use original style | ||
dartName = avoidInitialUnderscore(value.name); | ||
} | ||
|
||
dartNames[value.name] = disambiguateName( | ||
avoidInitialUnderscore(value.name), | ||
dartName, |
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.
If the ProtobufEnumStyle is not defined, I keep the orignal way of doing to not incorporate breaking change
class ProtobufEnumStyleParser implements SingleOptionParser { | ||
bool protobufEnumStyleEnabled = false; | ||
|
||
@override | ||
void parse(String name, String? value, OnError onError) { | ||
if (value != null) { | ||
onError('Invalid protobuf-enum-style option. No value expected.'); | ||
return; | ||
} | ||
protobufEnumStyleEnabled = true; | ||
} | ||
} | ||
|
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.
New argument to enable protobuf enum style parsing. By default, the value is false to not incorporate breaking change!
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.
Adding a new golden test to validate protobuf enum style
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.
Adding a new golden test to validate protobuf enum style
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.
Adding a new golden test to validate protobuf enum style
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.
Adding a new golden test to validate protobuf enum style
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.
Adding a new golden test to validate that we keep the old behavior if the argument is not set
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.
Adding a new golden test to validate that we keep the old behavior if the argument is not set
This PR changes the enum generation to convert protobuf enum definition following the protobuf style guideline to respect dart naming convention at the end.
It is essential to take into account that people will follow the Protobuf style guideline and naming convention and that developer will expect the dart protoc plugin to match the dart naming conventions.
#372 Also motivates this change.
I tried to keep everything as compatible as possible. To not expose breaking change, I hide the new enum generation under an option (false by default)
protobuf-enum-style
.Close #372