Introduce Error Prone #712
Conversation
Also handled ignored return values.
Also bump Guava -> `25.1-jre`
Conflicts: client/src/test/java/io/spine/client/ColumnFiltersShould.java ext.gradle server/src/main/java/io/spine/server/entity/storage/EntityColumn.java server/src/test/java/io/spine/server/entity/storage/ColumnTypeRegistryShould.java server/src/test/java/io/spine/server/storage/RecordStorageShould.java
Conflicts: client/src/test/java/io/spine/client/ColumnFiltersShould.java ext.gradle server/src/main/java/io/spine/server/entity/storage/EntityColumn.java server/src/test/java/io/spine/server/entity/storage/ColumnTypeRegistryShould.java server/src/test/java/io/spine/server/storage/RecordStorageShould.java
Codecov Report
@@ Coverage Diff @@
## master #712 +/- ##
============================================
+ Coverage 93.24% 93.32% +0.07%
- Complexity 3014 3033 +19
============================================
Files 369 371 +2
Lines 10349 10390 +41
Branches 676 678 +2
============================================
+ Hits 9650 9696 +46
+ Misses 504 500 -4
+ Partials 195 194 -1 |
alexander-yevsyukov
requested review from
armiol and
dmdashenkov
and removed request for
armiol
Conflicts: client/src/test/java/io/spine/client/ColumnFiltersShould.java client/src/test/java/io/spine/client/CommandFactoryShould.java client/src/test/java/io/spine/client/QueriesTest.java client/src/test/java/io/spine/client/QueryBuilderShould.java client/src/test/java/io/spine/client/QueryFactoryShould.java client/src/test/java/io/spine/core/CommandsTest.java ext.gradle server/src/main/java/io/spine/server/aggregate/Aggregate.java server/src/main/java/io/spine/server/entity/TransactionalEntity.java server/src/test/java/io/spine/server/bc/BoundedContextShould.java server/src/test/java/io/spine/server/entity/RecordBasedRepositoryShould.java server/src/test/java/io/spine/server/storage/RecordStorageShould.java
|
@dmdashenkov, PTAL. |
|
|
||
| assertEquals(3, FluentIterable.from(filter) | ||
| .size()); | ||
| assertEquals(3, StreamSupport.stream(filter.spliterator(), false) |
There was a problem hiding this comment.
Can we join the stream of commands (see above) and this one?
Also, please use Stream.of(...) instead of ImmutableList.of(...).stream().
| boolean noEvents = record.getEventList() | ||
| .isEmpty(); | ||
| if (noEvents && snapshotIsNotSet) { | ||
| throw new IllegalStateException("AggregateStateRecord instance should have either " |
There was a problem hiding this comment.
Please use Exceptions.
There was a problem hiding this comment.
No need here as we're not passing arguments to a formatted string.
| return Optional.absent(); | ||
| } | ||
|
|
||
| while (historyBackward.hasNext() |
There was a problem hiding this comment.
Please extract the loop into a separate method.
| @@ -353,8 +353,8 @@ protected CommandDispatcherRegistry registry() { | |||
| * the current runtime environment. | |||
| */ | |||
| private static boolean detectThreadsAllowed() { | |||
| final boolean appEngine = ServerEnvironment.getInstance() | |||
| .isAppEngine(); | |||
| boolean appEngine = ServerEnvironment.getInstance() | |||
There was a problem hiding this comment.
We're now allowed to spawn threads on AppEngine. I believe this is irrelevant now.
There was a problem hiding this comment.
This matter needs more attention across the whole codebase. Please file an issue for this and link it into this PR.
| return result; | ||
| } | ||
|
|
||
| static boolean hasNullableReturn(Method getter) { |
There was a problem hiding this comment.
Maybe, mayReturnNull(Method)?
| && getter.getParameterTypes().length == 0, | ||
| "Method `%s` is not a getter.", getter); | ||
| checkArgument(getAnnotatedVersion(getter).isPresent(), | ||
| format("Entity column getter should be annotated with `%s`.", |
There was a problem hiding this comment.
Please remove redundant format, as checkArgument is capable of string formatting on its own.
| Class<?> returnType = getter.getReturnType(); | ||
| Class<?> wrapped = wrap(returnType); | ||
| checkArgument(Serializable.class.isAssignableFrom(wrapped), | ||
| format("Cannot create column of non-serializable type %s by method %s.", |
| .collect(Collectors.toSet()); | ||
| Set<TenantId> tenants = Streams.stream(events) | ||
| .map(Events::getTenantId) | ||
| .collect(Collectors.toSet()); |
There was a problem hiding this comment.
Please add a static import for toSet().
| return aggregateStateRecord; | ||
| List<Event> expectedEvents = records.stream() | ||
| .map(TO_EVENT) | ||
| .collect(Collectors.toList()); |
There was a problem hiding this comment.
Please add a static import for toList().
| return eventMessage.equals(phase.getEventMessage()) | ||
| && event.getContext() | ||
| .equals(phase.getContext()) | ||
| && phase.isSuccessful() == successful; |
There was a problem hiding this comment.
Please split the expression into 3 with 3 local variables to make it more debuggable.
Also improved a method name.
Conflicts: client/src/test/java/io/spine/core/CommandsTest.java client/src/test/java/io/spine/grpc/StreamObserversTest.java core/src/test/java/io/spine/change/BooleanMismatchShould.java core/src/test/java/io/spine/change/DoubleMismatchShould.java core/src/test/java/io/spine/change/FloatMismatchShould.java core/src/test/java/io/spine/change/IntMismatchShould.java core/src/test/java/io/spine/change/LongMismatchShould.java core/src/test/java/io/spine/change/StringMismatchShould.java
|
@dmdashenkov, PTAL. |
There was a problem hiding this comment.
@alexander-yevsyukov, LGTM in general.
Let's discuss the remaining comments vocally if needed.
| fiveSecondsAgo) | ||
| .filter(wereWithinPeriod(minutesAgo(3), secondsAgo(10)) | ||
| ::apply) | ||
| .collect(toList()); | ||
|
|
||
| assertEquals(3, StreamSupport.stream(filter.spliterator(), false) | ||
| .count()); |
There was a problem hiding this comment.
This is still strange.
First, we generate a stream, then convert it to list and then back to stream, and, at last, obtain count().
| @Test | ||
| @DisplayName("consider command not scheduled when no scheduling options are present") | ||
| void recognizeNotScheduled() { | ||
| final Command cmd = requestFactory.createCommand(StringValue.getDefaultInstance()); |
There was a problem hiding this comment.
Why do we use final here again?
This PR adds Error Prone to the project and replaces
@Nullablefromjavax.annotationswith the one from the Checker Framework.It also starts eliminating the
finalqualifier for the local variables, since under Java 8 they are effectively final. We also want to improve the readability in the light of introduction of thevardeclaration in Java 10. So, this request also introduced an improved layout for calling builder methods.For information about the
varandfinalmatters, please see this page.Spine version advances to
0.10.41-SNAPSHOT.