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

Conversation

dansanduleac
Copy link
Contributor

After this PR

==COMMIT_MSG==
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.

Fixes #69.
==COMMIT_MSG==

Possible downsides?

@changelog-app
Copy link

changelog-app bot commented Nov 12, 2019

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

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.

Check the box to generate changelog(s)

  • Generate changelog entry

@@ -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.

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.

Copy link
Contributor

@ferozco ferozco left a comment

Choose a reason for hiding this comment

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

LGTM

}
}

@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

if (state.column() + thisWidth <= maxWidth) {
return state.withColumn(state.column() + (int) thisWidth)
.withLevelState(this, ImmutableLevelState.of(true));
Optional<State> oneLine = tryToFitOnOneLine(maxWidth, state);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Optional<State> oneLine = tryToFitOnOneLine(maxWidth, state);
return tryToFitOnOneLine(maxWidth, state)
.orElseGet(() -> state.updateAfterLevel(getBreakBehaviour().match(new BreakImpl(commentsHelper, maxWidth, state));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep looks better, tho I kept the orElse bit on two lines as I think it looks cleaner

@ferozco
Copy link
Contributor

ferozco commented Nov 12, 2019

👍

@bulldozer-bot bulldozer-bot bot merged commit d68b600 into develop Nov 12, 2019
@bulldozer-bot bulldozer-bot bot deleted the ds/limit-dots branch November 12, 2019 19:35
@svc-autorelease
Copy link
Collaborator

Released 0.3.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hard to read long method chains
3 participants