Improve ImmutableMultimapSerializer.

[ghost]

• Add support for serializing ImmutableSetMultimap, which is missing.
• Add explicit support for serializing ImmutableListMultimap
  (ImmutableMap delegates to ImmutableListMultimap in most cases, so it
  was implicitly supported).
• Add tests for ImmutableSetMultimap & ImmutableListMultimap, and
  improve existing tests.

[coveralls]

Coverage increased (+0.1%) to 85.185% when pulling fb396ee on 3xp0n3nt:improve-immutable-multimap into f0659ca on magro:master.

[coveralls]

Coverage increased (+0.1%) to 85.185% when pulling fb396ee on 3xp0n3nt:improve-immutable-multimap into f0659ca on magro:master.

[coveralls]

Coverage increased (+0.1%) to 85.185% when pulling fb396ee on 3xp0n3nt:improve-immutable-multimap into f0659ca on magro:master.

[magro]

Great, thanks for the PR! Do you think it makes sense to update the README related to this? Otherwise I'd be happy to merge as it is.

[ghost]

@magro No, I thought about it, but ImmutableMultiMapSerializer should have already been serializing ImmutableSetMultimap in the first place. Also, there are other Guava serializers that serialize specialized types in the same class, none of them mention that in the README. I feel that the design of the README would need to change to accommodate such details, by allowing a paragraph or so for each serializer class, explaining exactly what is and isn't supported, gotchas, etc. Improving the granularity of the README is a mini project in itself, and should be a separate issue, in my opinion. As it is, there is only a small, vague, blurb for each class, so this additional information would be out of place on its own.

[coveralls]

Coverage increased (+0.1%) to 85.185% when pulling e096e26 on 3xp0n3nt:improve-immutable-multimap into c29d105 on magro:master.

[ghost]

@magro Another idea I considered would have been to create a separate serializer class for ImmutableSetMultimap, which would eliminate the type checking, but I guess it just didn't feel like the right approach. Think about it like this: ImmutableMultimap already delegates by default internally to ImmtuableListMultimap. Do we pretend not to know this and create a separate serializer for ImmutableListMultimap as well? This could create confusion, if people don't know the internal workings of ImmutableMultimap, they would want to know why there is a separate ImmutableSetMultimapSerializer, but no ImmutableListMultimapSerializer. We could create both, but then again, other serializers don't split these up, so it would be deviating from the overall library design, and it would make ImmutableMultimapSerializer redundant, as ImmutableMultimap already delegates to ImmutableListMultimap.

The reason for the type checking is due to a design quirk in ImmutableMultimap. The nested Builder class will always create an ImmutableListMultimap when calling ImmutableMultimap#builder, so even if you are deserializing an explicit ImmutableListMultimap subtype, it will just work to call ImmtuableMultimap#builder (no requirement to call ImmutableListMultimap#builder - which is how it currently works without this PR), but if you are serializing an ImmtuableSetMultimap, you MUST call ImmutableSetMultimap#builder, which requires checking the type when deserializing to get the correct builder.

Personally, for my use case, I don't want to worry about what concrete implementation might be used by another developer in another module. For example, I register ImmutableMultimapSerializer in a common module, shared by client & server modules, communicating via KryoNet. I don't want to worry if the server developer is going to use an ImmutableSetMultimap, ImmutableListMultimap, or any other subtype. I would prefer one registration for the base type, and expect it to cover all the concrete implementations. Also, I don't want serializer registration explosion - having to register one class per subtype. So I think the library succeeds for the most part in this design, except in cases like this where the support for ImmutableSetMultimap should have been there originally, but wasn't, leading to a breakage later when another developer on my project decided to use an ImmutableSetMultimap in a network-serializable class.

I would prefer stricter acceptance guidelines when new serializers are being PR'd - making sure all subtypes are accounted for explicitly with testing. (Of course, there will be exceptions, but those should _known_ and then noted accordingly in the README.)

[ghost]

After writing my thesis up there, I realized that when a serializer class deals with subtypes in the same class, it is usually internal subtypes - at least for Guava - that are not exposed via a public API, so it's not really an issue in those cases. However, for API types, there are actually separate serializers, e.g. for ImmutableList & ImmutableSet, LinkedHashMultimapSerializer & LinkedListMultimapSerializer, etc. So maybe there is some precedent for creating a separate serializer for ImmutableSetMultimap.

@magro Let me know how you'd like to proceed. I have no problem adjusting the PR if you want to go that route.

[coveralls]

Coverage increased (+0.1%) to 85.185% when pulling eeb8393 on 3xp0n3nt:improve-immutable-multimap into c29d105 on magro:master.

[magro]

Thanks for the explanation, all makes sense!

Regarding PR acceptance guidelines: I can completely understand this! Unfortunately I can only do basic checking but don't have the time to look into it / do reviews as I'd do them during my day job (other OSS projects and "life" need some time as well). The alternative for me would be to significantly slow down PR acceptance or don't do anything on it at all. Therefore I'm following the "better done than perfect" attitude here, hoping that OSS will still lead to a better quality in the long term. In that sense, thanks a lot for your current contributions! :-) I'd also be happy to have more people help maintaining kryo and kryo-serializers... :-)

Back to the code: I don't have a clear preference which way to go, therefore I'd merge it as it is. Ok?

[ghost]

@magro Yeah, I understand. I'm fine with merging as-is! And you're very welcome!

[magro]

Thanks!

[ghost]

@magro No problem! Could I get a release for this?

[magro]

Of course, until tomorrow, today on travels.

[ghost]

@magro Sure, thanks a lot!

[magro]

kryo-serialzers 0.41 is just on its way to maven central.

[ghost]

@magro Thanks again!

[ghost]

@magro Hmm, the latest version in Maven Central is 0.38 - shouldn't 0.39 and 0.40 have already been there, at least? I don't know how long it takes, but I wouldn't think more than a few hours.

[ghost]

It shows up here: https://repo1.maven.org/maven2/de/javakaffee/kryo-serializers/ but not here: https://mvnrepository.com/artifact/de.javakaffee/kryo-serializers

Update: Also shows up here http://search.maven.org/#search%7Cgav%7C1%7Cg%3A%22de.javakaffee%22%20AND%20a%3A%22kryo-serializers%22

Not sure what's wrong with mvnrepository.com. Not going to use it anymore.

[magro]

Yeah, mvnrepository always takes some time to catch up, the repo itself is the truth :-)

[magro]

A better alternative: http://search.maven.org/#search%7Cga%7C1%7Cg%3A%22de.javakaffee%22%20AND%20a%3Akryo-serializers