From c4b467d50bd6bb52935075b438f602ff902db167 Mon Sep 17 00:00:00 2001 From: google-java-format Team Date: Sat, 6 Apr 2024 03:02:26 -0700 Subject: [PATCH] Support overlapping --lines ranges in google-java-format. The strict behavior comes from the underlying ImmutableRangeSet.Builder class, which does not allow overlapping ranges to be added. Let's use TreeRangeSet instead. I have also updated the documentation for the --lines flag to make it clear that the line numbers are a closed-closed interval. Tested with: ``` blaze run :google_java_format_binary -- --lines=1:5 --lines=3:8 - < Foo.java ``` Before this CL the command fails with: ``` '--lines=1:5' '--lines=3:8' - Overlapping ranges not permitted but found [0..5) overlapping [2..8) ``` After this CL the command succeeds. PiperOrigin-RevId: 622410610 --- .../googlejavaformat/java/CommandLineOptions.java | 8 +++++--- .../java/CommandLineOptionsParser.java | 4 ++-- .../googlejavaformat/java/UsageException.java | 2 +- .../java/CommandLineOptionsParserTest.java | 15 ++++++--------- 4 files changed, 14 insertions(+), 15 deletions(-) diff --git a/core/src/main/java/com/google/googlejavaformat/java/CommandLineOptions.java b/core/src/main/java/com/google/googlejavaformat/java/CommandLineOptions.java index 5a233284a..d5480a790 100644 --- a/core/src/main/java/com/google/googlejavaformat/java/CommandLineOptions.java +++ b/core/src/main/java/com/google/googlejavaformat/java/CommandLineOptions.java @@ -16,6 +16,8 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableRangeSet; +import com.google.common.collect.RangeSet; +import com.google.common.collect.TreeRangeSet; import java.util.Optional; /** @@ -178,7 +180,7 @@ static Builder builder() { static class Builder { private final ImmutableList.Builder files = ImmutableList.builder(); - private final ImmutableRangeSet.Builder lines = ImmutableRangeSet.builder(); + private final RangeSet lines = TreeRangeSet.create(); private final ImmutableList.Builder offsets = ImmutableList.builder(); private final ImmutableList.Builder lengths = ImmutableList.builder(); private boolean inPlace = false; @@ -204,7 +206,7 @@ Builder inPlace(boolean inPlace) { return this; } - ImmutableRangeSet.Builder linesBuilder() { + RangeSet linesBuilder() { return lines; } @@ -282,7 +284,7 @@ CommandLineOptions build() { return new CommandLineOptions( files.build(), inPlace, - lines.build(), + ImmutableRangeSet.copyOf(lines), offsets.build(), lengths.build(), aosp, diff --git a/core/src/main/java/com/google/googlejavaformat/java/CommandLineOptionsParser.java b/core/src/main/java/com/google/googlejavaformat/java/CommandLineOptionsParser.java index f7c3dec95..52a5e05d4 100644 --- a/core/src/main/java/com/google/googlejavaformat/java/CommandLineOptionsParser.java +++ b/core/src/main/java/com/google/googlejavaformat/java/CommandLineOptionsParser.java @@ -18,8 +18,8 @@ import com.google.common.base.CharMatcher; import com.google.common.base.Splitter; -import com.google.common.collect.ImmutableRangeSet; import com.google.common.collect.Range; +import com.google.common.collect.RangeSet; import java.io.IOException; import java.io.UncheckedIOException; import java.nio.file.Files; @@ -157,7 +157,7 @@ private static String getValue(String flag, Iterator it, String value) { * number. Line numbers are {@code 1}-based, but are converted to the {@code 0}-based numbering * used internally by google-java-format. */ - private static void parseRangeSet(ImmutableRangeSet.Builder result, String ranges) { + private static void parseRangeSet(RangeSet result, String ranges) { for (String range : COMMA_SPLITTER.split(ranges)) { result.add(parseRange(range)); } diff --git a/core/src/main/java/com/google/googlejavaformat/java/UsageException.java b/core/src/main/java/com/google/googlejavaformat/java/UsageException.java index a10f2d076..50d55d4d4 100644 --- a/core/src/main/java/com/google/googlejavaformat/java/UsageException.java +++ b/core/src/main/java/com/google/googlejavaformat/java/UsageException.java @@ -56,7 +56,7 @@ final class UsageException extends Exception { " --set-exit-if-changed", " Return exit code 1 if there are any formatting changes.", " --lines, -lines, --line, -line", - " Line range(s) to format, like 5:10 (1-based; default is all).", + " Line range(s) to format, e.g. the first 5 lines are 1:5 (1-based; default is all).", " --offset, -offset", " Character offset to format (0-based; default is all).", " --length, -length", diff --git a/core/src/test/java/com/google/googlejavaformat/java/CommandLineOptionsParserTest.java b/core/src/test/java/com/google/googlejavaformat/java/CommandLineOptionsParserTest.java index 08fbbbab3..93dfb79c2 100644 --- a/core/src/test/java/com/google/googlejavaformat/java/CommandLineOptionsParserTest.java +++ b/core/src/test/java/com/google/googlejavaformat/java/CommandLineOptionsParserTest.java @@ -16,7 +16,6 @@ import static com.google.common.truth.Truth.assertThat; import static java.nio.charset.StandardCharsets.UTF_8; -import static org.junit.Assert.fail; import com.google.common.collect.ImmutableList; import com.google.common.collect.Range; @@ -151,15 +150,13 @@ public void setExitIfChanged() { .isTrue(); } - // TODO(cushon): consider handling this in the parser and reporting a more detailed error @Test - public void illegalLines() { - try { - CommandLineOptionsParser.parse(Arrays.asList("-lines=1:1", "-lines=1:1")); - fail(); - } catch (IllegalArgumentException e) { - assertThat(e).hasMessageThat().contains("overlap"); - } + public void mergedLines() { + assertThat( + CommandLineOptionsParser.parse(Arrays.asList("-lines=1:5", "-lines=2:8")) + .lines() + .asRanges()) + .containsExactly(Range.closedOpen(0, 8)); } @Test