Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Limit the max width of method chain dots #70

Merged
merged 9 commits into from
Nov 12, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions changelog/@unreleased/pr-70.v2.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
type: improvement
improvement:
description: |-
Limit how far dots may appear in long method chains to 80 chars.

Note that this doesn't currently apply to prefixes (such as `foo.bar().baz().stream()`) because prefixes are also used to group together fully qualified class names and it's a bit trickier to handle that case.
links:
- https://github.com/palantir/palantir-java-format/pull/70
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import com.palantir.javaformat.doc.DocBuilder;
import com.palantir.javaformat.doc.Level;
import java.util.Optional;
import java.util.OptionalInt;
import org.immutables.value.Value;
import org.immutables.value.Value.Default;

Expand All @@ -28,20 +29,29 @@
*/
@Value.Immutable
public abstract class OpenOp implements Op {
/** The extra indent inside this level. */
public abstract Indent plusIndent();

/**
* When this level doesn't fit on one line, controls whether this level is to be broken (its breaks taken) or
* partially inlined onto the current line.
*/
@Default
public BreakBehaviour breakBehaviour() {
return BreakBehaviours.breakThisLevel();
}

/** If it's the last level of its parent, when to inline this level rather than break the parent. */
@Default
public LastLevelBreakability breakabilityIfLastLevel() {
return LastLevelBreakability.ABORT;
}

public abstract Optional<String> debugName();

/** Custom max column limit that contents of this level <em>before the last break</em> may not exceed. */
public abstract OptionalInt columnLimitBeforeLastBreak();

/**
* Make an ordinary {@code OpenOp}.
*
Expand Down Expand Up @@ -69,7 +79,7 @@ public static Op make(

@Override
public void add(DocBuilder builder) {
builder.open(plusIndent(), breakBehaviour(), breakabilityIfLastLevel(), debugName());
builder.open(this);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've also taken the opportunity of passing through OpenOp right into Level rather than unpacking its properties through multiple methods, as it was getting laborious to add new fields.

}

public static Builder builder() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@

package com.palantir.javaformat.doc;

import com.google.common.base.MoreObjects;
import com.google.common.collect.Range;
import com.google.errorprone.annotations.Immutable;
import com.palantir.javaformat.CommentsHelper;
Expand All @@ -25,25 +24,19 @@
import com.palantir.javaformat.Output;
import com.palantir.javaformat.doc.State.BreakState;
import java.util.Optional;
import org.immutables.value.Value;

/** A leaf node in a {@link Doc} for an optional break. */
@Immutable
public final class Break extends Doc implements Op {
private final FillMode fillMode;
private final String flat;
private final Indent plusIndent;
private final Optional<BreakTag> optTag;

private Break(FillMode fillMode, String flat, Indent plusIndent, Optional<BreakTag> optTag) {
this.fillMode = fillMode;
this.flat = flat;
this.plusIndent = plusIndent;
this.optTag = optTag;
}
@Value.Immutable
public abstract class Break extends Doc implements Op {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I converted this to an immutable thinking I wanted to add new fields to it but ended up not doing that.
Still, I prefer this style and it makes it easier to change in the future.

public abstract FillMode fillMode();

public FillMode getFillMode() {
return fillMode;
}
public abstract String flat();

public abstract Indent plusIndent();

public abstract Optional<BreakTag> optTag();

/**
* Make a {@code Break}.
Expand All @@ -54,7 +47,7 @@ public FillMode getFillMode() {
* @return the new {@code Break}
*/
public static Break make(FillMode fillMode, String flat, Indent plusIndent) {
return new Break(fillMode, flat, plusIndent, /* optTag= */ Optional.empty());
return builder().fillMode(fillMode).flat(flat).plusIndent(plusIndent).build();
}

/**
Expand All @@ -67,7 +60,7 @@ public static Break make(FillMode fillMode, String flat, Indent plusIndent) {
* @return the new {@code Break}
*/
public static Break make(FillMode fillMode, String flat, Indent plusIndent, Optional<BreakTag> optTag) {
return new Break(fillMode, flat, plusIndent, optTag);
return builder().fillMode(fillMode).flat(flat).plusIndent(plusIndent).optTag(optTag).build();
}

/**
Expand All @@ -76,21 +69,16 @@ public static Break make(FillMode fillMode, String flat, Indent plusIndent, Opti
* @return the new forced {@code Break}
*/
public static Break makeForced() {
return make(FillMode.FORCED, "", Indent.Const.ZERO);
return builder().fillMode(FillMode.FORCED).flat("").plusIndent(Indent.Const.ZERO).build();
}

/**
* Return the {@code Break}'s extra indent.
*
* @return the extra indent
* @param state
*/
public int evalPlusIndent(State state) {
return plusIndent.eval(state);
}

Indent getPlusIndent() {
return plusIndent;
return plusIndent().eval(state);
}

/**
Expand All @@ -99,7 +87,7 @@ Indent getPlusIndent() {
* @return whether the {@code Break} is forced
*/
public boolean isForced() {
return fillMode == FillMode.FORCED;
return fillMode() == FillMode.FORCED;
}

@Override
Expand All @@ -108,22 +96,22 @@ public void add(DocBuilder builder) {
}

@Override
float computeWidth() {
return isForced() ? Float.POSITIVE_INFINITY : (float) flat.length();
protected float computeWidth() {
return isForced() ? Float.POSITIVE_INFINITY : (float) flat().length();
}

@Override
String computeFlat() {
return flat;
protected String computeFlat() {
return flat();
}

@Override
Range<Integer> computeRange() {
protected Range<Integer> computeRange() {
return EMPTY_RANGE;
}

public State computeBreaks(State stateIn, boolean broken) {
State state = optTag.map(breakTag -> stateIn.breakTaken(breakTag, broken)).orElse(stateIn);
State state = optTag().map(breakTag -> stateIn.breakTaken(breakTag, broken)).orElse(stateIn);
return state.withBreak(this, broken);
}

Expand All @@ -143,17 +131,13 @@ public void write(State state, Output output) {
output.append(state, "\n", EMPTY_RANGE);
output.indent(breakState.newIndent());
} else {
output.append(state, flat, range());
output.append(state, flat(), range());
}
}

@Override
public String toString() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

return MoreObjects.toStringHelper(this)
.add("fillMode", fillMode)
.add("flat", flat)
.add("plusIndent", plusIndent)
.add("optTag", optTag)
.toString();
public static class Builder extends ImmutableBreak.Builder {}

public static Builder builder() {
return new Builder();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -69,21 +69,21 @@ final Range<Integer> range() {
*
* @return the width, or {@code Float.POSITIVE_INFINITY} if it must be broken
*/
abstract float computeWidth();
protected abstract float computeWidth();

/**
* Compute the {@code Doc}'s flat value. Not defined (and never called) if contains forced breaks.
*
* @return the flat value
*/
abstract String computeFlat();
protected abstract String computeFlat();

/**
* Compute the {@code Doc}'s {@link Range} of {@link Input.Token}s.
*
* @return the {@link Range}
*/
abstract Range<Integer> computeRange();
protected abstract Range<Integer> computeRange();

/**
* Make breaking decisions for a {@code Doc}.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,20 +17,16 @@
package com.palantir.javaformat.doc;

import com.google.common.base.MoreObjects;
import com.palantir.javaformat.BreakBehaviour;
import com.palantir.javaformat.BreakBehaviours;
import com.palantir.javaformat.Indent;
import com.palantir.javaformat.LastLevelBreakability;
import com.palantir.javaformat.Op;
import com.palantir.javaformat.OpenOp;
import com.palantir.javaformat.OpsBuilder;
import java.util.ArrayDeque;
import java.util.List;
import java.util.Optional;

/** A {@code DocBuilder} converts a sequence of {@link Op}s into a {@link Doc}. */
public final class DocBuilder {
private final Level base = Level.make(
Indent.Const.ZERO, BreakBehaviours.breakThisLevel(), LastLevelBreakability.ABORT, Optional.of("root"));
private final Level base = Level.make(OpenOp.builder().plusIndent(Indent.Const.ZERO).debugName("root").build());
private final ArrayDeque<Level> stack = new ArrayDeque<>();

/**
Expand Down Expand Up @@ -68,20 +64,9 @@ public DocBuilder withOps(List<Op> ops) {
return this;
}

/**
* Open a new {@link Level}.
*
* @param plusIndent the extra indent for the {@link Level}
* @param breakBehaviour how to decide whether to break this level or not
* @param breakabilityIfLastLevel if last level, when to break this rather than parent
* @param debugName
*/
public void open(
Indent plusIndent,
BreakBehaviour breakBehaviour,
LastLevelBreakability breakabilityIfLastLevel,
Optional<String> debugName) {
Level level = Level.make(plusIndent, breakBehaviour, breakabilityIfLastLevel, debugName);
/** Open a new {@link Level}. */
public void open(OpenOp openOp) {
Level level = Level.make(openOp);
stack.addLast(level);
}

Expand Down
Loading