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

add serializers for mutable Guava multimaps #57

Merged
merged 6 commits into from
Oct 4, 2016

Conversation

mtbc
Copy link
Contributor

@mtbc mtbc commented Aug 15, 2016

Provides serializers and accompanying tests for various Google Guava multimaps.

Does not attempt Kryo.copy support nor serialization of TreeMultimaps' comparators.

@coveralls
Copy link

coveralls commented Aug 15, 2016

Coverage Status

Coverage increased (+1.3%) to 83.46% when pulling d31b58b on mtbc:serialize-guava-multimaps into 9a4f71e on magro:master.

private static final boolean IMMUTABLE = false;

public ArrayListMultimapSerializer() {
super(DOES_NOT_ACCEPT_NULL, IMMUTABLE);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm copying the existing classes here but I'm confused: in ImmutableMapSerializer, etc., this DOES_NOT_ACCEPT_NULL is passed for Serializer's constructor's acceptsNull argument, which seems a bit backward? The names and values may need flipping, but the integration tests pass. 😕

Copy link
Owner

Choose a reason for hiding this comment

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

Haven't checked it, is this null case even tested?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My tests do try serializing null values.

@magro
Copy link
Owner

magro commented Aug 18, 2016

Thanks for the PR! I'm currently/still on vacation, will have a deeper look at it when I'm back (in 2 weeks).

Early feedback:

  • perhaps have serializers in a single java file (tests as well)?
  • could you update the readme by mentioning the new serializers?

@coveralls
Copy link

coveralls commented Aug 19, 2016

Coverage Status

Coverage increased (+1.3%) to 83.46% when pulling a04cbda on mtbc:serialize-guava-multimaps into 9a4f71e on magro:master.

@mtbc
Copy link
Contributor Author

mtbc commented Aug 19, 2016

I think the separate base classes do a lot for reuse and decluttering, though could probably move each with default visibility into the actual file of one of their concrete extensions if that would be preferable.

No hurry on my part, enjoy your vacation and good luck with catchup afterward. 😃

@magro magro merged commit e1ea315 into magro:master Oct 4, 2016
@magro
Copy link
Owner

magro commented Oct 4, 2016

Thanks for the PR, I just released it with 0.39.

@magro
Copy link
Owner

magro commented Oct 4, 2016

Btw, I also just updated the kryo dependency to 4.0.0 and released kryo-serializers 0.40 with this.

@mtbc mtbc deleted the serialize-guava-multimaps branch October 5, 2016 07:30
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