-
Notifications
You must be signed in to change notification settings - Fork 504
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 assoc. consts in enums representing protobuf enum aliases, resolves #792 #849
base: master
Are you sure you want to change the base?
Conversation
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.
This changes makes sense to me.
- I think the PR title should be something like:
Add support for enum aliases
. - You need to add some tests for the new use cases.
- Does prost need to check for
allow_alias
option? Ifprotoc
throw an error whenallow_alias
is not set, then we don't have to.
prost-build/src/code_generator.rs
Outdated
self.push_indent(); | ||
self.buf.push_str("/// Aliases.\n"); | ||
} | ||
for variant in &aliases { |
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.
Personally, I would place the for loop inside the if-block
prost-build/src/code_generator.rs
Outdated
|
||
if aliases.len() > 0 { | ||
self.push_indent(); | ||
self.buf.push_str("/// Aliases.\n"); |
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.
Three slashes is for rustdoc comments. This is not a doc comment, right?
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.
right
if !numbers.insert(value.number()) { | ||
continue; | ||
for m in &mappings { | ||
if m.proto_number == value.number() { |
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.
genereated_names
can be used to do this lookup in one step, right?
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 don't see how, that would require a lookup by value instead of name, no?
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.
You are right. The HashMap is the other way around. Please disregard my comment.
panic!("Generated enum variant names overlap: `{}` variant name to be used both by `{}` and `{}` ProtoBuf enum values", | ||
generated_variant_name, old_v, value.name()); | ||
// if alias ends up being a duplicate, we don't need it, and can skip it. | ||
// TODO: check if enum values are actually the same |
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 this TODO needs to be solved
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 feel like duplicates should always be disallowed. Even if they are an alias, then overlap is not disirable.
if !numbers.insert(value.number()) { | ||
continue; | ||
for m in &mappings { | ||
if m.proto_number == value.number() { |
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.
You are right. The HashMap is the other way around. Please disregard my comment.
panic!("Generated enum variant names overlap: `{}` variant name to be used both by `{}` and `{}` ProtoBuf enum values", | ||
generated_variant_name, old_v, value.name()); | ||
// if alias ends up being a duplicate, we don't need it, and can skip it. | ||
// TODO: check if enum values are actually the same |
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 feel like duplicates should always be disallowed. Even if they are an alias, then overlap is not disirable.
This PR adds associated constants to enum
impl
s representing enum aliases in protocol buffer enums as shown below. The aliases can be referenced by prost when used as default values, causing compilation to fail, see #792.