Skip to content

Commit

Permalink
Use compact-no-array style for @InlineMe annotations
Browse files Browse the repository at this point in the history
Suggestions by the `InlineMeSuggester` always use the array syntax for import annotations even when only one import is listed.

This clashes with checkstyle's [`AnnotationUseStyleCheck`](https://checkstyle.sourceforge.io/apidocs/com/puppycrawl/tools/checkstyle/checks/annotation/AnnotationUseStyleCheck.html) check which uses `COMPACT_NO_ARRAY` style per default and disallows single-value arrays. Note that the proposed style would also work with the two other checkstyle options.

Example:

```diff
- @InlineMe(replacement = "REPLACEMENT", "imports = {"java.time.Duration"})
+ @InlineMe(replacement = "REPLACEMENT", "imports = "java.time.Duration")
```

Note that this is a more nitty suggestion -- fine to drop this if the other style is preferred. But I believe this would reduce friction for consumers using checkstyle.

Fixes #2366

COPYBARA_INTEGRATE_REVIEW=#2366 from fawind:fw/inlineme-compact-no-array 6af0ad6
PiperOrigin-RevId: 376748570
  • Loading branch information
fawind authored and Error Prone Team committed Jun 1, 2021
1 parent 81695fd commit 9aa812b
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 13 deletions.
Expand Up @@ -83,7 +83,11 @@ String buildAnnotation() {
}

private static String quote(Set<String> imports) {
return "{\"" + Joiner.on("\", \"").join(imports) + "\"}";
String quoted = "\"" + Joiner.on("\", \"").join(imports) + "\"";
if (imports.size() == 1) {
return quoted;
}
return "{" + quoted + "}";
}

// TODO(glorioso): be tolerant of trailing semicolon
Expand Down
Expand Up @@ -42,6 +42,15 @@ public void testBuildAnnotation_withImports() {
+ "imports = {\"java.time.Duration\", \"java.time.Instant\"})\n");
}

@Test
public void testBuildAnnotation_withSingleImport() {
assertThat(
InlineMeData.buildAnnotation(
"REPLACEMENT", ImmutableSet.of("java.time.Duration"), ImmutableSet.of()))
.isEqualTo(
"@InlineMe(replacement = \"REPLACEMENT\", " + "imports = \"java.time.Duration\")\n");
}

@Test
public void testInstanceMethodNewImport() {
refactoringTestHelper
Expand All @@ -67,7 +76,7 @@ public void testInstanceMethodNewImport() {
"public final class Client {",
" private Duration deadline = Duration.ofSeconds(5);",
" @InlineMe(replacement = \"this.setDeadline(Duration.ofMillis(millis))\","
+ " imports = {\"java.time.Duration\"})",
+ " imports = \"java.time.Duration\")",
" @Deprecated",
" public void setDeadline(long millis) {",
" setDeadline(Duration.ofMillis(millis));",
Expand Down Expand Up @@ -100,7 +109,7 @@ public void testStaticMethodInNewClass() {
"public final class Client {",
" @InlineMe(",
" replacement = \"Duration.ofMillis(millis)\", ",
" imports = {\"java.time.Duration\"})",
" imports = \"java.time.Duration\")",
" @Deprecated",
" public Duration fromMillis(long millis) {",
" return Duration.ofMillis(millis);",
Expand Down Expand Up @@ -142,7 +151,7 @@ public void testReturnField() {
"import com.google.errorprone.annotations.InlineMe;",
"import java.time.Duration;",
"public final class Client {",
" @InlineMe(replacement = \"Duration.ZERO\", imports = {\"java.time.Duration\"})",
" @InlineMe(replacement = \"Duration.ZERO\", imports = \"java.time.Duration\")",
" @Deprecated",
" public Duration getZero() {",
" return Duration.ZERO;",
Expand Down Expand Up @@ -230,7 +239,7 @@ public void testMethodReference() {
"import java.util.Optional;",
"public final class Client {",
" @InlineMe(replacement = \"input.map(Duration::ofMillis)\", ",
" imports = {\"java.time.Duration\"})",
" imports = \"java.time.Duration\")",
" @Deprecated",
" public Optional<Duration> silly(Optional<Long> input) {",
" return input.map(Duration::ofMillis);",
Expand Down Expand Up @@ -258,7 +267,7 @@ public void testNewClass() {
"import com.google.errorprone.annotations.InlineMe;",
"import org.joda.time.Instant;",
"public final class Client {",
" @InlineMe(replacement = \"new Instant()\", imports = {\"org.joda.time.Instant\"})",
" @InlineMe(replacement = \"new Instant()\", imports = \"org.joda.time.Instant\")",
" @Deprecated",
" public Instant silly() {",
" return new Instant();",
Expand Down Expand Up @@ -286,7 +295,7 @@ public void testNewArray() {
"import com.google.errorprone.annotations.InlineMe;",
"import org.joda.time.Instant;",
"public final class Client {",
" @InlineMe(replacement = \"new Instant[42]\", imports = {\"org.joda.time.Instant\"})",
" @InlineMe(replacement = \"new Instant[42]\", imports = \"org.joda.time.Instant\")",
" @Deprecated",
" public Instant[] silly() {",
" return new Instant[42];",
Expand Down Expand Up @@ -314,7 +323,7 @@ public void testNewNestedClass() {
"import com.google.errorprone.annotations.InlineMe;",
"public final class Client {",
" @InlineMe(replacement = \"new NestedClass()\", ",
" imports = {\"com.google.frobber.Client.NestedClass\"})",
" imports = \"com.google.frobber.Client.NestedClass\")",
" @Deprecated",
" public NestedClass silly() {",
" return new NestedClass();",
Expand Down Expand Up @@ -499,7 +508,7 @@ public void testWithCast() {
"import java.time.Duration;",
"public final class Client {",
" @InlineMe(replacement = \"this.foo((Duration) duration)\", imports ="
+ " {\"java.time.Duration\"})",
+ " \"java.time.Duration\")",
" @Deprecated",
" public void setDuration(Object duration) {",
" foo((Duration) duration);",
Expand Down Expand Up @@ -626,7 +635,7 @@ public void testTernaryOverMultipleLines() {
"public final class Client {",
" @InlineMe(replacement = \""
+ "deadline.compareTo(Duration.ZERO) > 0 ? deadline : Duration.ZERO\", ",
"imports = {\"java.time.Duration\"})",
"imports = \"java.time.Duration\")",
" @Deprecated",
" public Duration getDeadline(Duration deadline) {",
" return deadline.compareTo(Duration.ZERO) > 0",
Expand Down Expand Up @@ -660,7 +669,7 @@ public void testStaticCallingAnotherQualifiedStatic() {
"import java.time.Duration;",
"public final class Client {",
" @InlineMe(replacement = \"Client.getDeadline2()\", ",
" imports = {\"com.google.frobber.Client\"})",
" imports = \"com.google.frobber.Client\")",
" @Deprecated",
" public static Duration getDeadline() {",
" return Client.getDeadline2();",
Expand Down Expand Up @@ -692,7 +701,7 @@ public void staticReferenceToJavaLang() {
"import com.google.errorprone.annotations.InlineMe;",
"public final class Client {",
" @InlineMe(replacement = \"format(template, arg)\", staticImports ="
+ " {\"java.lang.String.format\"})",
+ " \"java.lang.String.format\")",
" @Deprecated",
" public static String myFormat(String template, String arg) {",
" return format(template, arg);",
Expand Down Expand Up @@ -723,7 +732,7 @@ public void replacementContainsGenericInvocation() {
"import java.util.List;",
"public final class Client {",
" @InlineMe(replacement = \"new ArrayList<Void>()\", imports ="
+ " {\"java.util.ArrayList\"})",
+ " \"java.util.ArrayList\")",
" @Deprecated",
" public static List<Void> newArrayList() {",
" return new ArrayList<Void>();",
Expand Down

0 comments on commit 9aa812b

Please sign in to comment.