-
Notifications
You must be signed in to change notification settings - Fork 40
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
feat(generator/dart): emit enums #1082
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1082 +/- ##
=======================================
Coverage 95.89% 95.89%
=======================================
Files 34 34
Lines 1365 1365
=======================================
Hits 1309 1309
Misses 56 56 ☔ View full report in Codecov by Sentry. |
That depends on the transport. Over HTTP+JSON (which is what we are targeting for Rust) the enums are indeed strings. Over gRPC + Protobuf the enums are ints. For most services we want to target HTTP+JSON. See go/cloud-sdk-new-languages and go/coryan:the-next-8. Even for services where we use gRPC and Protobuf: this usage should remain hidden from application developers.
Your call then, I have no idea what would be considered "better" in Dart-world.
I would discourage that: consider what happens when a library adds a type that creates a conflict. We wish to make such changes non-breaking. If we get clever with the names the change would be breaking I think. |
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 think we need to change these to use strings. Otherwise, the code is looking good. Nice tests too.
/// The state of a | ||
/// [SecretVersion][google.cloud.secretmanager.v1.SecretVersion], indicating if | ||
/// it can be accessed. |
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.
FYI: eventually we will need to define these cross-reference comments, the output will need to be something like:
/// The state of a | |
/// [SecretVersion][google.cloud.secretmanager.v1.SecretVersion], indicating if | |
/// it can be accessed. | |
/// The state of a | |
/// [SecretVersion][google.cloud.secretmanager.v1.SecretVersion], indicating if | |
/// it can be accessed. | |
/// | |
/// [google.cloud.secretmanager.v1.SecretVersion]: /* the Dart way to link to the `SecretVersion` class */ |
There is a lot of prior art in the Rust codec, and probably some opportunities for refactoring.
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.
+1, I'd like to clean up the comments here (for dart, called doc comments).
Assuming the reference can be resolved in the namespace, this would be emitted as:
/// The state of a [SecretVersion], indicating if it can be accessed.
If we can't resolve the reference we'd do something like emit 'SecretVersion' in a code-fence - not tell the dart doc tool to try and resolve the ref.
return messageName(e.Parent.Parent) + "_" + name | ||
} | ||
return name | ||
return strcase.ToLowerCamel(e.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.
TIL: Dart supports nested constants.
Gotcha - I updated to use the string value (
Makes sense - preferring to generate stable output, and reducing unnecessary breaking API changes. |
Update the dart generator to emit enums:
This uses Dart classes to represent enums instead of actual language
enum
s. I assume this will work better with unknown values coming over the wire. And I'm assuming enum values over the wire will be represented by their int values (and not something like their SCREAMING_CAPS string name).I'm using
'$'
to concatenate nested messages and enums.'_'
and''
are also reasonable; nothing's particularly idiomatic. Perhaps shorter names could be used if we determine there are no namespace conflicts in a library?