-
-
Notifications
You must be signed in to change notification settings - Fork 107
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
feat: avoid breaking on certain method chains and arguments #632
feat: avoid breaking on certain method chains and arguments #632
Conversation
7496bae
to
56a1f93
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @jtkiesel !
There is one issue with formatting method invocation with lambda, but otherwise, seems to be working fine !
If possible, it would be great to extract some functions at the beginning of the expression method, as this method brings a lot of cognitive load
shouldDedent: | ||
// dedent when simple method invocation | ||
countMethodInvocation !== 1 && | ||
// dedent when (chain) method invocation | ||
ctx.primaryPrefix[0] && | ||
ctx.primaryPrefix[0].children.fqnOrRefType && | ||
!( | ||
ctx.primaryPrefix[0].children.fqnOrRefType[0].children.Dot !== | ||
undefined | ||
) && | ||
// indent when lambdaExpression | ||
ctx.primarySuffix[0].children.methodInvocationSuffix && | ||
ctx.primarySuffix[0].children.methodInvocationSuffix[0].children | ||
.argumentList && | ||
ctx.primarySuffix[0].children.methodInvocationSuffix[0].children | ||
.argumentList[0].children.expression && | ||
ctx.primarySuffix[0].children.methodInvocationSuffix[0].children | ||
.argumentList[0].children.expression[0].children | ||
.lambdaExpression === undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's great we could get rid of this part !
const newExpression = | ||
ctx.primaryPrefix[0].children.newExpression?.[0].children; | ||
const arrayCreationExpression = | ||
newExpression?.arrayCreationExpression?.[0].children; | ||
const classInstanceCreationExpression = | ||
newExpression?.unqualifiedClassInstanceCreationExpression?.[0].children; | ||
const isBreakableNewExpression = | ||
countMethodInvocation <= 1 && | ||
[ | ||
arrayCreationExpression?.classOrInterfaceType?.[0].children.classType[0] | ||
.children.typeArguments, | ||
arrayCreationExpression?.arrayCreationExplicitInitSuffix?.[0].children | ||
.arrayInitializer[0].children.variableInitializerList, | ||
classInstanceCreationExpression?.classOrInterfaceTypeToInstantiate[0] | ||
.children.typeArgumentsOrDiamond?.[0].children.typeArguments, | ||
classInstanceCreationExpression?.argumentList | ||
].some(breakablePart => breakablePart); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We maybe could extract a isBreakableNewExpression function to lighten the cognitive load on this function ?
?.map(suffix => suffix.children.methodInvocationSuffix?.[0].children) | ||
.find(methodInvocationSuffix => methodInvocationSuffix); | ||
const isCapitalizedIdentifier = | ||
nextToLastIdentifier && /^\p{Lu}/u.test(nextToLastIdentifier); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use Uppercase_Letter instead of Lu, I found it more explicit ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also be in favor of extracting a method here, even if it is a small one
const fqnOrRefType = | ||
ctx.primaryPrefix[0].children.fqnOrRefType?.[0].children; | ||
const fqnOrRefTypeParts = [ | ||
fqnOrRefType?.fqnOrRefTypePartFirst[0], | ||
...(fqnOrRefType?.fqnOrRefTypePartRest ?? []) | ||
]; | ||
const nextToLastIdentifier = | ||
fqnOrRefTypeParts[fqnOrRefTypeParts.length - 2]?.children | ||
.fqnOrRefTypePartCommon[0].children.Identifier?.[0].image; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDYT of extracting a nextToLastIdentifier function ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since I extracted isCapitalizedIdentifier
out into its own function and nextToLastIdentifier
was only used there, I simply moved it into that function. Let me know if you'd still like that broken out further.
void singleLambdaWithBlockLastArgument() { | ||
a.of(b, c, d, e -> { | ||
return f; | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is an issue with formatting this snippet
Input:
class T {
public void method() {
a.of(
aaaaaaaaaaaaaaaaaaaaaaaaaa,
bbbbbbbbbbbbbbbbbbbbbbbbbb,
cccccccccccccccccccccccccc,
e -> {
return f;
}
);
}
}
Output:
class T {
public void method() {
a.of(aaaaaaaaaaaaaaaaaaaaaaaaaa,
bbbbbbbbbbbbbbbbbbbbbbbbbb,
cccccccccccccccccccccccccc,
e -> {
return f;
});
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching this! I was able to fix this one without much issue. However, it lead me to find another that has me stumped at the moment:
class T {
void singleLambdaWithBlockLastArgumentAndLongLambdaArguments() {
a.of(b,
(
cccccccccccccccccccccccccc,
dddddddddddddddddddddddddd,
eeeeeeeeeeeeeeeeeeeeeeeeee
) -> {
return f;
}
);
}
}
I can't use ifBreak
to conditionally break before b,
if the lambda's argument list breaks, because ifBreak
requires the provided groupId
to be one that has already been printed (i.e. it must be a groupId
that appears before the ifBreak
). The only solution I've thought of so far is grouping the lambda's argument list with the "parent" argument list (in this case, b
), so that the lambda argument list breaking also breaks the "parent" argument list. However, that seems rather ugly and I'm not even sure if something like that is possible without changing the grammar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was able to resolve both of these. The latter required some additional research, but looking into Prettier's own source code, I realized they were solving this for JS/TS by using conditionalGroup
, and I was able to do the same.
.find(methodInvocationSuffix => methodInvocationSuffix); | ||
const isCapitalizedIdentifier = | ||
nextToLastIdentifier && /^\p{Lu}/u.test(nextToLastIdentifier); | ||
const shouldBreakBeforeFirstMethodInvocation = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the end, I ended up with something like that, WDYT ?
...
const { newExpression, nextToLastIdentifier, firstMethodInvocation } = extract(ctx);
const isCapitalizedIdentifier = isCapitalized(nextToLastIdentifier);
const shouldBreakBeforeFirstMethodInvocation =
countMethodInvocation > 1 &&
!(isCapitalizedIdentifier) &&
firstMethodInvocation !== undefined;
...
PS: the name of the extract method should be changed :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up solving this via individual functions. Let me know what you think.
56a1f93
to
29e631a
Compare
@clementdessoude I was able to solve the formatting issues & refactored to reduce the cognitive complexity a bit. I was also able to simplify a few things in the process. Let me know if you think the |
What changed with this PR:
Method chains and arguments are now broken more similarly to Prettier JavaScript. Method invocations assumed to be static (i.e. those on capitalized identifiers) are now never broken, short method chains prefer to be broken on arguments instead, and argument lists containing a single lambda with body at the end prefer not to break.
Example
Input
Output
Relative issues or prs:
Closes #626