Fix/no guava immutable recipes #258
Conversation
|
|
||
| public abstract class AbstractNoGuavaImmutableOf extends Recipe { | ||
|
|
||
| private transient final MethodMatcher IMMUTABLE_MATCHER = new MethodMatcher(getGuavaType() + " of(..)"); |
There was a problem hiding this comment.
What's the background behind adding the transient here? Is it because of the call to getGuavaType when combined with final?
I'm reading up a bit and found some conflicting reports of using transient, final, and non-trivial objects together when serialization might be involved.
If you think it'll work fine that fine by me; but wanted to ask both to learn and get a second opinion.
There was a problem hiding this comment.
The RewriteTests validates that a recipe can be serializable and also checks that after deserializing are equal.
MethodMatcher was causing an InvalidDefinitionException with jackson. I've seen in many other recipes that they put all this "complex" fields to static members (because it makes sense for performance, but also avoids the serde problem) but I cannot do that since it depends on a method that must be defined by an extending class...
I've also seen some recipes with transient + final (I don't know if wrongly used), and after checking it myself in the place where the serialization+deserialization is done, it seems to not be an issue with jackson.
WDYT? I can move this to a method (although it will be instantiating a new one every time) or remove the final if you want.
There was a problem hiding this comment.
Maybe it would make sense to define a constructor in AbstractNoGuavaImmutableOf that takes in two arguments, and reduce NoGuavaImmutableMapOf to just calling that constructor with those two arguments?
There was a problem hiding this comment.
It's a good idea the constructor thing, but I do not see how it will solve the matcher issue
There was a problem hiding this comment.
we could move the creation of the matcher into getVisitor(), but before the return Preconditions.check. I don't think that getVisitor gets called repeatedly nowadays, but I could be wrong.
There was a problem hiding this comment.
pushed just now in 20564ab
hope you agree; otherwise open for a revert and further discussion
What's changed?
First, we removed a previous change that allowed Map.of to be assigned to ImmutableMap (this was done to allow nested maps), but was causing uncompilable code.
Added some tests to validate that we do not produce changes in those scenarios for all three collections (List, Set, Map).
Also, since all three recipes (List, Set, Map), have the same duplicated code that needs to be synched, and already had slightly different versions, I've decided to extract it in a parent class, to avoid future inconsistence problems like it was happening right now
What's your motivation?
Fixes #256
Anything in particular you'd like reviewers to focus on?
Checklist
./gradlew licenseFormat