Skip to content

Add an exception for extendedFieldName #763

Open
Mr14huashao wants to merge 3 commits intoEsotericSoftware:masterfrom
Mr14huashao:kryo-699
Open

Add an exception for extendedFieldName #763
Mr14huashao wants to merge 3 commits intoEsotericSoftware:masterfrom
Mr14huashao:kryo-699

Conversation

@Mr14huashao
Copy link
Copy Markdown

@Mr14huashao Mr14huashao commented Aug 29, 2020

Add a test case about boolean field for #699

@Mr14huashao Mr14huashao mentioned this pull request Aug 29, 2020
Closed
@theigl
Copy link
Copy Markdown
Collaborator

theigl commented Aug 30, 2020

@Mr14huashao: As I said in #759, we need a solution with minimal overhead. Ideally, the duplicate field check would be done only once when the serializer is first initialized. I think we should only log a warning so we do not break existing applications and force them to use extendedFieldNames.

theigl
theigl requested changes Aug 30, 2020
View reviewed changes
Comment thread src/com/esotericsoftware/kryo/serializers/CompatibleFieldSerializer.java Outdated
Comment thread src/com/esotericsoftware/kryo/serializers/CompatibleFieldSerializer.java Outdated
Comment thread src/com/esotericsoftware/kryo/serializers/CompatibleFieldSerializer.java Outdated
Comment thread test/com/esotericsoftware/kryo/serializers/CompatibleFieldSerializerTest.java Outdated
@Mr14huashao
Copy link
Copy Markdown
Author

Mr14huashao commented Aug 31, 2020

@theigl Thank you for checking my code.
Based on the suggestion you provided, I modified some code, and the code is better.
I've benefited a lot from it.
Thank you very much.

@Mr14huashao Mr14huashao requested a review from theigl September 17, 2020 07:30
theigl
theigl requested changes Sep 17, 2020
View reviewed changes
Copy link
Copy Markdown
Collaborator

@theigl theigl left a comment

Choose a reason for hiding this comment

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

@Mr14huashao: Sorry for the delay. I just reviewed your PR.

Please adjust your tests to use existing support from KryoTestCase and change your tests to look like others in the same file.

Comment thread test/com/esotericsoftware/kryo/serializers/CompatibleFieldSerializerTest.java Outdated
Comment thread test/com/esotericsoftware/kryo/serializers/CompatibleFieldSerializerTest.java Outdated
Comment thread test/com/esotericsoftware/kryo/serializers/CompatibleFieldSerializerTest.java Outdated
Comment thread test/com/esotericsoftware/kryo/serializers/CompatibleFieldSerializerTest.java Outdated
Comment thread test/com/esotericsoftware/kryo/serializers/CompatibleFieldSerializerTest.java Outdated
Comment thread src/com/esotericsoftware/kryo/serializers/CompatibleFieldSerializer.java Outdated
Comment thread src/com/esotericsoftware/kryo/serializers/CompatibleFieldSerializer.java Outdated
Comment thread src/com/esotericsoftware/kryo/serializers/CompatibleFieldSerializer.java Outdated
Comment thread src/com/esotericsoftware/kryo/serializers/CompatibleFieldSerializer.java Outdated
Comment thread src/com/esotericsoftware/kryo/serializers/CompatibleFieldSerializer.java Outdated
@Mr14huashao Mr14huashao mentioned this pull request Sep 19, 2020
Closed
@Mr14huashao Mr14huashao requested a review from theigl September 21, 2020 11:08
theigl
theigl reviewed Sep 22, 2020
View reviewed changes
Comment thread src/com/esotericsoftware/kryo/serializers/CompatibleFieldSerializer.java Outdated
theigl
theigl reviewed Sep 22, 2020
View reviewed changes
Comment thread test/com/esotericsoftware/kryo/serializers/CompatibleFieldSerializerTest.java Outdated
@Mr14huashao Mr14huashao requested a review from theigl October 19, 2020 01:28
@theigl
Copy link
Copy Markdown
Collaborator

theigl commented Nov 2, 2020

@Mr14huashao: I'm currently trying to stabilize Kryo 5.0.0 and will merge only bug fix PRs in the near future. Let's revisit this after Kryo 5.0.1 has been released.

@Mr14huashao
Copy link
Copy Markdown
Author

Mr14huashao commented Nov 26, 2020

@theigl Do I want to modify some content based on the current version?

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

Reviewers

@theigl theigl Awaiting requested review from theigl

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Mr14huashao @theigl