TheNeuralBit commented on a change in pull request #15648:
URL: https://github.com/apache/beam/pull/15648#discussion_r728372285
##########
File path:
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/HL7v2IO.java
##########
@@ -163,11 +163,6 @@
return new
AutoValue_HL7v2IO_Write.Builder().setHL7v2Store(StaticValueProvider.of(hl7v2Store));
}
- /** Write HL7v2 Messages to a store. */
- private static Write.Builder write(ValueProvider<String> hl7v2Store) {
- return new AutoValue_HL7v2IO_Write.Builder().setHL7v2Store(hl7v2Store);
- }
Review comment:
I think it's possible some of these functions were intended to be
public, but given that they're not tested (otherwise they would be used), and
no one has asked about them, I agree with the decision to ahead and delete them.
##########
File path:
sdks/java/testing/load-tests/src/main/java/org/apache/beam/sdk/loadtests/JobFailure.java
##########
@@ -19,7 +19,6 @@
import static java.lang.String.format;
import static java.util.Optional.empty;
-import static java.util.Optional.of;
Review comment:
Could you make this change for `Optional.empty` and `String.format` as
well? It looks like the `BadImport` check doesn't catch these, but they're in
the same spirit.
##########
File path:
sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/reflect/DoFnSignatures.java
##########
@@ -2396,6 +2396,7 @@ ErrorReporter forParameter(ParameterDescription param) {
"parameter of type %s at index %s", format(param.getType()),
param.getIndex()));
}
+ @SuppressWarnings("AnnotateFormatMethod")
Review comment:
Why do we need to suppress this here?
##########
File path:
runners/core-construction-java/src/test/java/org/apache/beam/runners/core/construction/CommonCoderTest.java
##########
@@ -514,20 +511,4 @@ private void verifyDecodedValue(CommonCoder coder, Object
expectedValue, Object
throw new IllegalStateException("Unknown coder URN: " + coder.getUrn());
}
}
-
- /**
- * Utility for adding new entries to the common coder spec -- prints the
serialized bytes of the
- * given value in the given context using JSON-escaped strings.
- */
- private static <T> String jsonByteString(Coder<T> coder, T value, Context
context)
Review comment:
Could you keep this one around and suppress the warning? It looks like a
development utility.
##########
File path:
sdks/java/core/src/test/java/org/apache/beam/sdk/coders/BigDecimalCoderTest.java
##########
@@ -39,6 +39,7 @@
private static final Coder<BigDecimal> TEST_CODER = BigDecimalCoder.of();
+ @SuppressWarnings("BigDecimalLiteralDouble")
Review comment:
Could you change these to `BigDecimal.valueOf` isntead of suppressing?
That's what's suggested at
https://errorprone.info/bugpattern/BigDecimalLiteralDouble
##########
File path:
runners/core-construction-java/src/main/java/org/apache/beam/runners/core/construction/renderer/PortablePipelineDotRenderer.java
##########
@@ -104,8 +102,8 @@ private void exitBlock() {
indent -= 4;
}
- @FormatMethod
- private void writeLine(@FormatString String format, Object... args) {
+ @SuppressWarnings("AnnotateFormatMethod")
+ private void writeLine(String format, Object... args) {
Review comment:
It looks like you couldn't get the `@FormatMethod` annotations to work,
what went wrong here?
If it is something we can't solve, it would be preferable to just keep the
global exception rather than enabling the exception and suppressing it
everywhere.
##########
File path:
runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/GaugeData.java
##########
@@ -63,7 +63,7 @@ public GaugeResult extractResult() {
public static class EmptyGaugeData extends GaugeData {
private static final EmptyGaugeData INSTANCE = new EmptyGaugeData();
- private static final Instant EPOCH = new
Instant(GlobalWindow.TIMESTAMP_MIN_VALUE);
+ private static final Instant EPOCH = GlobalWindow.TIMESTAMP_MIN_VALUE;
Review comment:
I think that constructing new instances of joda types here and elsewhere
was intentional. It was probably misguided though, based on
[docs](https://www.joda.org/joda-time/apidocs/org/joda/time/Instant.html)
`Instant` and `Duration` are immutable, so we shouldn't need to make a copy to
avoid side effects. Maybe @kennknowles or @lukecwik could confirm if this
change is safe?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
