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

Is there any reason not to use names of enum? #321

Closed
kwon37xi opened this issue Jun 22, 2015 · 4 comments
Closed

Is there any reason not to use names of enum? #321

kwon37xi opened this issue Jun 22, 2015 · 4 comments

Comments

@kwon37xi
Copy link
Contributor

When I see EnumSerializer, it does not use enum's name. It uses ordinal.

This makes a problem when the order of enum constants changed
When the order changed, if it throws an exception, it's ok.
But if the deserialized ordinal exists in the new enum class, kryo does not throw any exception, just sets wrong enum value.

I made EnumStringSerializer by myself, and want to contribute to this project.

But Kryo's enum support has been very long time. I'm not sure whether you have specific reason not to use names for a long time.

Is there any reason not to use enum's name?

@NathanSweet
Copy link
Member

Ordinals are smaller. We'd merge an Enum serializer that uses names, but it
won't be used by default. It should be called EnumNameSerializer.

On Mon, Jun 22, 2015 at 2:54 PM, KwonNam Son notifications@github.com
wrote:

When I see EnumSerializer, it does not use enum's name. It uses ordinal.

This makes a problem when the order of enum constants changed
When the order changed, if it throws an exception, it's ok.
But if the deserialized ordinal exists in the new enum class, kryo does
not throw any exception, just sets wrong enum value.

I made EnumStringSerializer by myself, and want to contribute to this
project.

But Kryo's enum support has been very long time. I'm not sure whether you
have specific reason not to use names for a long time.

Is there any reason not to use enum's name?


Reply to this email directly or view it on GitHub
#321.

@magro
Copy link
Collaborator

magro commented Jun 22, 2015

@NathanSweet Don't we need a replaceDefaultSerializer then?

@NathanSweet
Copy link
Member

Don't think so, EnumSerializer is lower priority than any default
serializer added by a user.

On Mon, Jun 22, 2015 at 6:02 PM, Martin Grotzke notifications@github.com
wrote:

@NathanSweet https://github.com/NathanSweet Don't we need a
replaceDefaultSerializer then?


Reply to this email directly or view it on GitHub
#321 (comment)
.

@magro
Copy link
Collaborator

magro commented Jun 22, 2015

Ah, right.

kwon37xi added a commit to kwon37xi/kryo that referenced this issue Jun 23, 2015
NathanSweet added a commit that referenced this issue Jun 23, 2015
@magro magro closed this as completed Mar 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants