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

Use @JsonProperty and lowercase feature when serializing Enums despite write using toString() #4039

Merged

Conversation

iProdigy
Copy link
Contributor

Serialization complement of #4036

@iProdigy
Copy link
Contributor Author

@JooHyukKim Could you also review this PR please?

*
* @since 2.16
*/
protected final EnumValues _valuesByToString;
Copy link
Member

Choose a reason for hiding this comment

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

I am not confident about readability perspective, but is it possible to use the same/similar structure as other EnumValues members' javadoc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Appreciate the review - can you elaborate on what you mean here?

Copy link
Member

Choose a reason for hiding this comment

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

I originally thought it will be easier to read if javadoc of _values, _valuesByEnumNaming, and _valuesByToString had similarly structured paragraph and use similar terms. But can skip my comment, thanks 👍🏻

@@ -82,6 +92,20 @@ public EnumSerializer(EnumValues v, Boolean serializeAsIndex, EnumValues valuesB
_values = v;
_serializeAsIndex = serializeAsIndex;
_valuesByEnumNaming = valuesByEnumNaming;
_valuesByToString = null;
Copy link
Member

Choose a reason for hiding this comment

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

Should just use this(v, serializeAsIndex, valuesByEnumNaming, null); ?

Also let's add @Deprecated (and Javadoc) here

@@ -71,6 +80,7 @@ public EnumSerializer(EnumValues v, Boolean serializeAsIndex)
_values = v;
_serializeAsIndex = serializeAsIndex;
_valuesByEnumNaming = null;
_valuesByToString = null;
Copy link
Member

Choose a reason for hiding this comment

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

Can just delegate (with this(v, serializeAsIndex, null, null)).

Also, unless called from non-deprecated code, let's also mark as @Deprecated // since 2.16.

Copy link
Member

@cowtowncoder cowtowncoder left a comment

Choose a reason for hiding this comment

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

Looks good; just need to add some more Deprecation markers & could chain from deprecated constructors.

Actually, I can do these post-merge, so approving & merging now.

@cowtowncoder cowtowncoder merged commit 3c48461 into FasterXML:2.16 Jul 15, 2023
3 checks passed
@cowtowncoder cowtowncoder changed the title feat(enum): use JsonProperty and lowercase feature despite write using toString Use @JsonProperty and lowercase feature when serializing Enums despite write using toString Jul 15, 2023
@cowtowncoder cowtowncoder changed the title Use @JsonProperty and lowercase feature when serializing Enums despite write using toString Use @JsonProperty and lowercase feature when serializing Enums despite write using toString() Jul 15, 2023
cowtowncoder added a commit that referenced this pull request Jul 15, 2023
@iProdigy iProdigy deleted the fix/enum-write-tostring-jsonproperty branch July 15, 2023 23:31
cowtowncoder added a commit that referenced this pull request Jul 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants