-
Notifications
You must be signed in to change notification settings - Fork 40
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
Clean up some style warnings #777
Conversation
@@ -61,12 +61,12 @@ public AddDocumentHandler(GlobalState globalState) { | |||
public StreamObserver<AddDocumentRequest> handle( | |||
StreamObserver<AddDocumentResponse> responseObserver) { | |||
return new StreamObserver<>() { | |||
Multimap<String, Future<Long>> futures = HashMultimap.create(); | |||
final Multimap<String, Future<Long>> futures = HashMultimap.create(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it that these variables are made final but some other variables in this same file are not?
I also prefer to avoid making function-scope variables final - I feel like it adds verbosity but not much value in most cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are not really function-scope variables, they are class variables for the StreamObserver
instance.
ObjectToCompositeFieldTransformer.enrichCompositeField(obj, compositeFieldValue); | ||
} | ||
} | ||
case IndexableFieldDef<?> fieldDef when fieldDef.hasDocValues() -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL switch expression doesn't need explicit instanceof, and also the matching with when
. I'm finding these large case blocks a bit odd, but I guess I can get used to these.
for (String completedMergeFile : copyState.getCompletedMergeFilesList()) { | ||
completedMergeFiles.add(completedMergeFile); | ||
} | ||
Set<String> completedMergeFiles = new HashSet<>(copyState.getCompletedMergeFilesList()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of these are pretty good, like this one might be better performant too (if the JIT was not optimizing it already).
Went through and fixed a bunch of style warnings from my IDE.