From 7aa830817d29524cd92fa1dd22e503e8292b196e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dan=20S=C4=83nduleac?= Date: Wed, 16 Oct 2019 14:20:05 +0100 Subject: [PATCH] Improvement: Keep assignments on one line too (#17) Inline prefix of initializer for variable and field assignments too, not just declarations. --- changelog/@unreleased/pr-17.v2.yml | 6 ++++ .../palantir/javaformat/BreakBehaviour.java | 13 +++++++++ .../com/palantir/javaformat/doc/Level.java | 29 ++++++++++++------- .../javaformat/java/JavaInputAstVisitor.java | 10 +++++-- .../javaformat/java/testdata/A.output | 7 ++--- .../javaformat/java/testdata/B20128760.output | 20 ++++++------- .../javaformat/java/testdata/B23708487.output | 11 ++++--- .../javaformat/java/testdata/B24202287.output | 21 +++++++------- 8 files changed, 72 insertions(+), 45 deletions(-) create mode 100644 changelog/@unreleased/pr-17.v2.yml diff --git a/changelog/@unreleased/pr-17.v2.yml b/changelog/@unreleased/pr-17.v2.yml new file mode 100644 index 000000000..d861060d2 --- /dev/null +++ b/changelog/@unreleased/pr-17.v2.yml @@ -0,0 +1,6 @@ +type: improvement +improvement: + description: "Inline prefix of initializer for variable and field assignments too, + not just declarations." + links: + - https://github.com/palantir/palantir-java-format/pull/17 diff --git a/palantir-java-format/src/main/java/com/palantir/javaformat/BreakBehaviour.java b/palantir-java-format/src/main/java/com/palantir/javaformat/BreakBehaviour.java index ff42d80f9..fdb0ccde6 100644 --- a/palantir-java-format/src/main/java/com/palantir/javaformat/BreakBehaviour.java +++ b/palantir-java-format/src/main/java/com/palantir/javaformat/BreakBehaviour.java @@ -13,6 +13,19 @@ public enum BreakBehaviour { * Break if by doing so all inner levels then fit on a single line. However, don't break if we can fit in the {@link * Doc docs} up to the first break (which might be nested inside the next doc if it's a {@link Level}), in order to * prevent exceeding the maxLength accidentally. + * + *

Whether we decide to break or not, the indent of this level is still followed. */ BREAK_ONLY_IF_INNER_LEVELS_THEN_FIT_ON_ONE_LINE, + /** + * Same as {@link #BREAK_ONLY_IF_INNER_LEVELS_THEN_FIT_ON_ONE_LINE}, but always ignoring this level's indent if we + * decide not to break it. + */ + BREAK_ONLY_IF_INNER_LEVELS_THEN_FIT_ON_ONE_LINE_IGNORING_INDENT, + ; + + public boolean isBreakOnlyIfInnerLevelsThenFitOnOneLine() { + return this == BREAK_ONLY_IF_INNER_LEVELS_THEN_FIT_ON_ONE_LINE + || this == BREAK_ONLY_IF_INNER_LEVELS_THEN_FIT_ON_ONE_LINE_IGNORING_INDENT; + } } diff --git a/palantir-java-format/src/main/java/com/palantir/javaformat/doc/Level.java b/palantir-java-format/src/main/java/com/palantir/javaformat/doc/Level.java index fff1aa9cf..b825b6f70 100644 --- a/palantir-java-format/src/main/java/com/palantir/javaformat/doc/Level.java +++ b/palantir-java-format/src/main/java/com/palantir/javaformat/doc/Level.java @@ -30,6 +30,7 @@ import java.util.ArrayList; import java.util.List; import java.util.Optional; +import java.util.stream.Collector; import java.util.stream.Collectors; /** A {@code Level} inside a {@link Doc}. */ @@ -40,6 +41,8 @@ public final class Level extends Doc { */ private static final int MAX_BRANCHING_COEFFICIENT = 7; + private static final Collector> GET_LAST_COLLECTOR = Collectors.reducing((u, v) -> v); + private final Indent plusIndent; // The extra indent following breaks. private final BreakBehaviour breakBehaviour; // Where to break when we can't fit on one line. private final Breakability breakabilityIfLastLevel; // If last level, when to break this rather than parent. @@ -152,7 +155,7 @@ public State computeBreaks(CommentsHelper commentsHelper, int maxWidth, State st // But undo the break in a special case, if the inner levels didn't fit on one line. // Note: this is currently only used for variable initialisers - if (breakBehaviour == BreakBehaviour.BREAK_ONLY_IF_INNER_LEVELS_THEN_FIT_ON_ONE_LINE) { + if (breakBehaviour.isBreakOnlyIfInnerLevelsThenFitOnOneLine()) { List innerLevels = this.docs.stream() .filter(doc -> doc instanceof Level) .map(doc -> ((Level) doc)) @@ -162,23 +165,26 @@ public State computeBreaks(CommentsHelper commentsHelper, int maxWidth, State st boolean prefixFits = false; if (anyLevelWasBroken) { - // Find the first level, skipping empty levels (that contain nothing, or are made up + // Find the last level, skipping empty levels (that contain nothing, or are made up // entirely of other empty levels). - Level firstLevel = innerLevels.stream() + + // Last level because there might be other in-between levels after the initial break like `new int[] + // {`, and we want to skip those. + Level lastLevel = innerLevels.stream() .filter(doc -> StartsWithBreakVisitor.INSTANCE.visit(doc) != Result.EMPTY) - .findFirst() + .collect(GET_LAST_COLLECTOR) .orElseThrow(() -> new IllegalStateException( "Levels were broken so expected to find at least a non-empty level")); - // Add the width of tokens, breaks before the firstLevel. We must always have space for + // Add the width of tokens, breaks before the lastLevel. We must always have space for // these. - List leadingDocs = docs.subList(0, docs.indexOf(firstLevel)); + List leadingDocs = docs.subList(0, docs.indexOf(lastLevel)); float leadingWidth = getWidth(leadingDocs); // Potentially add the width of prefixes we want to consider as part of the width that // must fit on the same line, so that we don't accidentally break prefixes when we could // have avoided doing so. - leadingWidth += new CountWidthUntilBreakVisitor(maxWidth - state.indent).visit(firstLevel); + leadingWidth += new CountWidthUntilBreakVisitor(maxWidth - state.indent).visit(lastLevel); boolean fits = !Float.isInfinite(leadingWidth) && state.column + leadingWidth <= maxWidth; @@ -190,10 +196,11 @@ public State computeBreaks(CommentsHelper commentsHelper, int maxWidth, State st // Allow long strings to stay on the same line, expecting that StringWrapper will // reflow them later. if (prefixFits || isSingleString()) { - // don't break this level, but preserve indentation - broken = - tryToLayOutLevelOnOneLine( - commentsHelper, maxWidth, state.withNoIndent().withIndentIncrementedBy(plusIndent)); + State newState = state.withNoIndent(); + if (breakBehaviour == BreakBehaviour.BREAK_ONLY_IF_INNER_LEVELS_THEN_FIT_ON_ONE_LINE) { + newState = newState.withIndentIncrementedBy(plusIndent); + } + broken = tryToLayOutLevelOnOneLine(commentsHelper, maxWidth, newState); } } diff --git a/palantir-java-format/src/main/java/com/palantir/javaformat/java/JavaInputAstVisitor.java b/palantir-java-format/src/main/java/com/palantir/javaformat/java/JavaInputAstVisitor.java index 811b8afac..7972d9f7f 100644 --- a/palantir-java-format/src/main/java/com/palantir/javaformat/java/JavaInputAstVisitor.java +++ b/palantir-java-format/src/main/java/com/palantir/javaformat/java/JavaInputAstVisitor.java @@ -602,7 +602,10 @@ public Void visitAssert(AssertTree node, Void unused) { @Override public Void visitAssignment(AssignmentTree node, Void unused) { sync(node); - builder.open(plusFour); + builder.open( + plusFour, + BreakBehaviour.BREAK_ONLY_IF_INNER_LEVELS_THEN_FIT_ON_ONE_LINE_IGNORING_INDENT, + Breakability.NO_PREFERENCE); scan(node.getVariable(), null); builder.space(); splitToken(operatorName(node)); @@ -621,7 +624,10 @@ public Void visitBlock(BlockTree node, Void unused) { @Override public Void visitCompoundAssignment(CompoundAssignmentTree node, Void unused) { sync(node); - builder.open(plusFour); + builder.open( + plusFour, + BreakBehaviour.BREAK_ONLY_IF_INNER_LEVELS_THEN_FIT_ON_ONE_LINE_IGNORING_INDENT, + Breakability.NO_PREFERENCE); scan(node.getVariable(), null); builder.space(); splitToken(operatorName(node)); diff --git a/palantir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/A.output b/palantir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/A.output index 3a2bc9214..dfcecbcc5 100644 --- a/palantir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/A.output +++ b/palantir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/A.output @@ -67,9 +67,8 @@ class A { arrayWithLongName[0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0] = arrayWithLongName[ 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0]; - something = - 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 - + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 - + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2; + something = 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2 + 2; } } diff --git a/palantir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/B20128760.output b/palantir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/B20128760.output index 8015a4996..31bb45ffa 100644 --- a/palantir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/B20128760.output +++ b/palantir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/B20128760.output @@ -1,8 +1,7 @@ class B20128760 { void f() { - x = - xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx( - xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx); + x = xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx( + xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx); XxxxxxxxxXxx.xxxxxxx() .xxx(XxxxxxxxxXxxXxxx.Xxxxx.XXXXX, "Xxxxx xxxxxxxxx xxxxx") @@ -25,14 +24,13 @@ class B20128760 { .xxxxxxxxXx("XxxxxxxxXxxxxxxxxXxx", xxxxxxxxXxxxxxxxxXxxXx, XX.xxxxxXx( XX.xxxxx(), XX.xxxxxx(XxxxxxxXxxxxxxxxxxXxxxxxxxx.xxxxx))); - xxXxxxxxxxXxxxXxxx = - (XxxxXxxx) - xxxxxxxxXxxxxxxxxx - .xxx() - .xxxxxXxxxxxx - .xxxXxxxxxxx(xxxxxxxxXxxxxxxxxx.xxx().xxxxXxxxx.xxxXxxxxxxXxxx() - 1) - .xxxXxxx() - .xxxxXxxxXxXx(X.xx.xxxxxxxxxxxxxxxxxxxxxxxxxxx); + xxXxxxxxxxXxxxXxxx = (XxxxXxxx) + xxxxxxxxXxxxxxxxxx + .xxx() + .xxxxxXxxxxxx + .xxxXxxxxxxx(xxxxxxxxXxxxxxxxxx.xxx().xxxxXxxxx.xxxXxxxxxxXxxx() - 1) + .xxxXxxx() + .xxxxXxxxXxXx(X.xx.xxxxxxxxxxxxxxxxxxxxxxxxxxx); xxxxxxXxxx( xxxxxxxxxx diff --git a/palantir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/B23708487.output b/palantir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/B23708487.output index 96be46e64..9a9be3737 100644 --- a/palantir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/B23708487.output +++ b/palantir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/B23708487.output @@ -1,10 +1,9 @@ class B23708487 { static { - TASK_AS_OWNER = - com.google.security.acl.proto2api.ACL.Entry.newBuilder() - .setRole(com.google.security.acl.proto2api.ACL.Entry.ROLE.OWNER.getNumber()) - .setScope(com.google.security.acl.proto2api.ACL.Scope.newBuilder() - .setMdbUser(BorgTaskInfo.getMdbUser().getMdbUserName())) - .build(); + TASK_AS_OWNER = com.google.security.acl.proto2api.ACL.Entry.newBuilder() + .setRole(com.google.security.acl.proto2api.ACL.Entry.ROLE.OWNER.getNumber()) + .setScope(com.google.security.acl.proto2api.ACL.Scope.newBuilder() + .setMdbUser(BorgTaskInfo.getMdbUser().getMdbUserName())) + .build(); } } diff --git a/palantir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/B24202287.output b/palantir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/B24202287.output index d4800def5..eeb004535 100644 --- a/palantir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/B24202287.output +++ b/palantir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/B24202287.output @@ -1,16 +1,15 @@ class B24202287 { { - this.overflowContactCompositeSupportedRenderers = - this.getSharePanelResponse - .contents - .unifiedSharePanelRenderer - .contents[0] - .connectionSection - .connectionsOverflowMenu - .connectionsOverflowMenuRenderer - .contents[0] - .overflowConnectionSectionRenderer - .contacts[0]; + this.overflowContactCompositeSupportedRenderers = this.getSharePanelResponse + .contents + .unifiedSharePanelRenderer + .contents[0] + .connectionSection + .connectionsOverflowMenu + .connectionsOverflowMenuRenderer + .contents[0] + .overflowConnectionSectionRenderer + .contacts[0]; int y = ((int[]) null)[0]; }