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

UnicodeBlockSerializer #70

Merged
merged 21 commits into from
Jun 14, 2017
Merged

UnicodeBlockSerializer #70

merged 21 commits into from
Jun 14, 2017

Conversation

Pr0methean
Copy link
Contributor

New serializer for Character.UnicodeBlock, with test. Takes into account
that this class is effectively an enum.

New serializer for Character.UnicodeBlock, with test. Takes into account
that this class is effectively an enum.
Remove use of diamond operator, to restore JDK6 compatibility
@ghost
Copy link

ghost commented Dec 25, 2016

@Pr0methean Thanks for the contribution. Perhaps next time you could do this work on a local branch, and once everything is working, squash, push, and pull request. The email spam for every one of these commits for all 31 watchers of this repository isn't very polite, thanks! 😉

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 85.333% when pulling 54272a5 on Pr0methean:master into d34f6f0 on magro:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 85.333% when pulling 54272a5 on Pr0methean:master into d34f6f0 on magro:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 85.333% when pulling 54272a5 on Pr0methean:master into d34f6f0 on magro:master.

@coveralls
Copy link

coveralls commented Dec 25, 2016

Coverage Status

Coverage increased (+0.08%) to 85.359% when pulling 8f74c32 on Pr0methean:master into d34f6f0 on magro:master.

@Pr0methean
Copy link
Contributor Author

@3xp0n3nt Will do. Sorry, I didn't see your comment until after I'd finished, and I didn't realize I'd left the PR open after the tests had failed.

@ghost
Copy link

ghost commented Dec 25, 2016

@Pr0methean No problem. 😺

@magro
Copy link
Owner

magro commented Dec 25, 2016

@Pr0methean thanks for the PR! I don't see any reason why this shouldn't go directly into kryo, as it's part of the jdk and not jvm/vendor specific. Can you please submit the PR against kryo?

Copy link
Owner

@magro magro left a comment

Choose a reason for hiding this comment

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

LGTM. Can you please add this serializer to the readme?

try {
assertSame(deserialize(kryo, serialized, UnicodeBlock.class),
UnicodeBlock.UNIFIED_CANADIAN_ABORIGINAL_SYLLABICS);
} catch (NullPointerException e) {
Copy link
Owner

Choose a reason for hiding this comment

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

Can be removed now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@coveralls
Copy link

coveralls commented Jun 13, 2017

Coverage Status

Coverage increased (+0.08%) to 85.359% when pulling 4098a90 on Pr0methean:master into d34f6f0 on magro:master.

@coveralls
Copy link

coveralls commented Jun 13, 2017

Coverage Status

Coverage increased (+0.08%) to 85.359% when pulling 0a31024 on Pr0methean:master into d34f6f0 on magro:master.

@Pr0methean
Copy link
Contributor Author

@magro magro merged commit c4ce10f into magro:master Jun 14, 2017
@magro
Copy link
Owner

magro commented Jun 14, 2017

Thanks!

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