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

Parsing Map<Enum,Obj> - 501 #1950

Merged
merged 4 commits into from Feb 21, 2022
Merged

Conversation

TheDoctorOne
Copy link
Contributor

@TheDoctorOne TheDoctorOne commented Sep 4, 2021

Related to issue #501.

Parsing "Map<Enum,Obj>" where enum's toString method is overridden and not returning the original name of the constant.
Test code and backtrace of the issue: #501 (comment)

With the added implementation, it first checks for name, as it was doing before. And if the result does not match with any constant that Enum has, it also checks for the output of the "toString()" method and returns the result of it.

@google-cla google-cla bot added the cla: yes label Sep 4, 2021
@Marcono1234
Copy link
Collaborator

Marcono1234 commented Sep 4, 2021

Relates to #1920, which is (partially) about toString() being used for map keys during serialization.

@Chaostical
Copy link

Chaostical commented Nov 23, 2021

****

@TheDoctorOne
Copy link
Contributor Author

TheDoctorOne commented Nov 23, 2021

Resolved the conflict caused by the variable name change from "field" to "constantField"

Copy link
Member

@eamonnmcmanus eamonnmcmanus left a comment

This seems reasonable. Just a small quibble about style.

@Marcono1234
Copy link
Collaborator

Marcono1234 commented Nov 23, 2021

Might be good to add a test for this, for example similar to what you have shown in #501 (comment).
Possibly also verifying that constant names have higher precedence, e.g.:

enum SwitchedToString {
  A("B"),
  B("A");

  private final String toString;

  SwitchedToString(String toString) {
    this.toString = toString;
  }

  @Override
  public String toString() {
    return toString;
  }
}

Note that I am not a member of this project so feel free to consider this only as suggestion.

Copy link
Contributor Author

@TheDoctorOne TheDoctorOne left a comment

Didn't know that, thx for the heads up

@TheDoctorOne TheDoctorOne requested a review from eamonnmcmanus Feb 15, 2022
@eamonnmcmanus
Copy link
Member

eamonnmcmanus commented Feb 21, 2022

Thanks! I've run this change against the tests for all of Google's internal code and I didn't find any problems.

@eamonnmcmanus eamonnmcmanus merged commit 7ee3e27 into google:master Feb 21, 2022
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants