Skip to content
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

fix getter/setter generated for optional enum fields should use option #1060

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

giangndm
Copy link

This PR fixed mismatch between getter/setter generated for optional enum fields and enum value i32 as described in #1027

Copy link
Collaborator

@caspermeijn caspermeijn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the name_fallback getter nessesary? You could do .name().unwrap_or_default() to get the same result, right? I think that will result in more idiomatic code.

Comment on lines +565 to 566
msg.privacy_level_4_fallback(),
default_enum_value::PrivacyLevel::PrivacyLevelprivacyLevelFour
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would turn this around to simplify the code.

Suggested change
msg.privacy_level_4_fallback(),
default_enum_value::PrivacyLevel::PrivacyLevelprivacyLevelFour
msg.privacy_level_4(),
Some(default_enum_value::PrivacyLevel::PrivacyLevelprivacyLevelFour)

Copy link
Collaborator

@caspermeijn caspermeijn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to see new test cases for an optional protobuf enum field.

match self.#ident {
#match_some
::core::option::Option::None => #default,
}
}

#[doc=#get_doc]
pub fn #get(&self) -> ::core::option::Option<#ty> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should only return an Option<T> for optional protobuf fields. I think it should remain the original code for non-optional fields

@giangndm
Copy link
Author

Why is the name_fallback getter nessesary? You could do .name().unwrap_or_default() to get the same result, right? I think that will result in more idiomatic code.

If we use unwrap_or_default, I don't think it can work with some custom default enum like this:

enum PrivacyLevel {
  PRIVACY_LEVEL_ONE = 1;
  PRIVACY_LEVEL_TWO = 2;
  PRIVACY_LEVEL_PRIVACY_LEVEL_THREE = 3;
  PRIVACY_LEVELPRIVACY_LEVEL_FOUR = 4;
}

message Test {
  optional PrivacyLevel privacy_level_1 = 1 [default = PRIVACY_LEVEL_ONE];
  optional PrivacyLevel privacy_level_3 = 2 [default = PRIVACY_LEVEL_PRIVACY_LEVEL_THREE];
  optional PrivacyLevel privacy_level_4 = 3 [default = PRIVACY_LEVELPRIVACY_LEVEL_FOUR];
}

Java implementation use has_ for checking and the field is set with default value in this case. How about think about has_ function?

Copy link
Collaborator

@caspermeijn caspermeijn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR should only change the generated code for optional enums. The generated code to non-optional enums should be unchanged.

I think optional enums should have two getters: fn #get(&self) -> Option<#ty> and fn #get _or_default(&self) -> #ty. I believe that will cover your usecase and be more correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants