Remove nullable annotation configuration in fix serialization. #621
Conversation
nimakarimipour
requested review from
msridhar and
lazaroclapp
and removed request for
msridhar
There was a problem hiding this comment.
Looks good to me and good refactoring/simplification. There is a missing comment to be deleted and a minor question, but otherwise looks good to me. Maybe worth testing this internally with a compatible version of the auto-annotator? Both to make sure all runs and to familiarize yourself with the process, @nimakarimipour .
| /** Stores information suggesting a type change of an element in source code. */ | ||
| public class SuggestedFixInfo { | ||
| /** Stores information suggesting adding @Nullable on an element in source code. */ | ||
| public class SuggestedNullableFixInfo { | ||
|
|
||
| /** FixLocation of the target element in source code. */ | ||
| private final FixLocation fixLocation; | ||
| /** Error which will be resolved by this type change. */ | ||
| private final ErrorMessage errorMessage; | ||
| /** Suggested annotation. */ |
There was a problem hiding this comment.
Remove this comment as well.
| /** Stores information suggesting a type change of an element in source code. */ | ||
| public class SuggestedFixInfo { | ||
| /** Stores information suggesting adding @Nullable on an element in source code. */ | ||
| public class SuggestedNullableFixInfo { |
There was a problem hiding this comment.
Why the rename here? It makes sense for now, but I wonder if we will have to flip it right back once we start adding other types of fixes? Or maybe the idea will be to add a common supertype for fixes and retain this one as a subclass?
There was a problem hiding this comment.
I did the renaming since instantiating this class only means we are suggesting Nullable and the value Nullable is hardcoded in this class. I though for better understanding the class purpose, I should refine the name.
There was a problem hiding this comment.
Sounds good for now. We might revisit this if we are adding other kinds of fixes, but right not it isn't clear if that will happen within NullAway or within the annotator or where, so this makes sense to me.
Thank you for your comment. Sure, will do that @lazaroclapp |
|
Hi @lazaroclapp, I checked this internally with a compatible version of Annotator. Everything is working correctly, this is ready for |
There was a problem hiding this comment.
LGTM.
Feel free to merge when/if test pass. But please make the commit title "Remove nullable annotation configuration in fix serialization" (rather than "Removes") and make sure the squashed commit description is the PR's description, not the list of individual commit messages, please :)
nimakarimipour
changed the title
Following GH-517 If
serializeFixMetadatais active,NullAwaywill serialize suggested type changes (currently just adding@Nullable). In theFixSerializationConfigfile we can set a custom annotation thatNullAwaywill use in the output serializations.However, this process can be simplified by just asking
NullAwayto serialize type change locations with a default annotation name, any tool processing this output can use their own preferred annotation.After this PR the following entity will be removed from the config
.xmlfile:And a sample out below:
Will be changed to simply: