-
Notifications
You must be signed in to change notification settings - Fork 28
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
impl(generator): handle nested messages and enums in Rust #78
impl(generator): handle nested messages and enums in Rust #78
Conversation
The mapping nested messages and enums uses a public module within the crate. That requires a new "snake case" accessor for the message name, as Rust uses snake case for modules. We also need a boolean to find out if the module is needed at all. And we need to get a different attribute for the fully qualified name of each message and enum, as we must refer to the qualified name of the generated struct when using the type.
ab25896
to
a23054a
Compare
return m.c.FQMessageName(m.s, m.state) | ||
} | ||
|
||
func (m *message) NameSnakeCase() string { |
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.
comment: all of these different case top level case conversion functions make me wonder if there is a better way to handle this and/or there is not enough flexibility at the template layer. I originally chose mustache because it is simple and does not allow you to code in the template itself. As time goes on we should monitor to make sure this is still the right choice. We could switch over to Go's template engine in the future to provide much more extensibility in the templates.
Nothing to change for now, but something we should think more about.
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.
Ack and agree.
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.
Nice, just a couple small things.
|
||
c := &Codec{} | ||
if got := c.MessageName(message, api.State); got != "Replication" { | ||
t.Errorf("mismatched message name, want=Replication, got=%s", got) |
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.
nit: goes convention for testing is backwards from most languages. I would usually expect these strings to be written as "got Replication, want %s"
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.
Fixed.
// The [SecretVersion][google.cloud.secretmanager.v1.SecretVersion] may be | ||
// accessed. | ||
pub const SecretVersion_Enabled: SecretVersion_State = SecretVersion_State(1); | ||
// Not specified. This value is unused and invalid. |
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.
Looks like we are missing an extra /
in the template.
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.
Agree, created #84
The mapping nested messages and enums uses a public module within the crate.
That requires a new "snake case" accessor for the message name, as Rust uses
snake case for modules.
We also need a boolean to find out if the module is needed at all.
And we need to get a different attribute for the fully qualified name of each
message and enum, as we must refer to the qualified name of the generated
struct when using the type.
Fixes #40. This PR is more bulky than I like, it snowballed on me. Let me know if you want me to break it into smaller PRs.