-
Notifications
You must be signed in to change notification settings - Fork 859
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
odd formatting for lambdas #19
Comments
I don't think the problem is lambda-specific, but this lambda-using code is doing a really good job of tripping up on the general issue. If we can solve that issue, ideally this code ought to be able to show up as Stream<ItemKey> itemIdsStream =
stream(members)
.flatMap(m ->
m.getFieldValues()
.entrySet()
.stream()
.filter(fv -> itemLinkFieldIds.contains(fv.getKey()))
.flatMap(fv ->
FieldDTO.deserializeStringToListOfStrings(fv.getValue())
.stream()
.map(id -> new ItemKey(fieldsById.get(fv.getKey()).getItemTypeId(), id)))); |
In addition to the general problem, do we also have something lambda-specific? Your example has the line break after A result of the current behavior is that any chained calls or arguments to the next expression are indented 1 space from the expression, rather than 4. This is probably to spec, and IIRC it also comes up with the ternary operator, but it looks even weirder here because it's 1 space, rather than the 3 we see with the ternary operator. I'm talking about lines like this one...
...(which kind of works out because
|
Yes, the issue of whether to break before or after -> is on our list of style guide issues to resolve before we move to Java 8. |
The formatter started always breaking after Anyway, the current behaviour is: x ->
y()
x -> {
y();
} Instead of: x
-> y()
x
-> {
y();
} I like the consistency between expression and statement lambdas, and it seems like the only way to support the suggested formatting of this example: Stream<ItemKey> itemIdsStream =
stream(members)
.flatMap(m ->
m.getFieldValues()
... |
The decision might not have been officially made, but it has been now. Keep |
Has the code been modified to reflect this decision? |
It has been modified to break after There should be another release soon, or you can build it at head to see the current behaviour. |
Hmm, I have some code which, before I tried applying google-java-format to it, I half-expected to end up looking like this:
but when I actually did format it (using the version built from 00530c0), it looked like this:
Is the current behaviour intentional? |
Has a decision been made about this issue? It's really annoying that the lambda parameter and the arrow token always begin a new line. For example, consider how
This would look much nicer as follows:
This problem is exacerbated in AOSP mode. Here's how
Lambdas in AOSP mode are indented 12 spaces! This creates a huge amount of unnecessary whitespace and inhibits readability, especially compared to using a traditional
Notice how in the above, the body of the lambda is indented only four spaces from the |
Hi @basil, I think you can workaround this nasty lambda indentation behaviour for a good number of cases in the meantime, by using method references[1] whenever possible. If I were to use the example you gave above as a template, then here's what I'd turn it into:
[1] Or refactoring the lambda expression into a private method, and calling it from a method reference or a new lambda... |
@jbduncan that may work in some individual cases, but unfortunately, it is not feasible to apply this workaround when reformatting a codebase which already makes heavy use of lambdas. That being said, it is certainly a useful workaround to apply on a case-by-case basis until this bug is resolved. |
google#19 The style guide says: "A line is never broken adjacent to the arrow in a lambda, except that a break may come immediately after the arrow if the body of the lambda consists of a single unbraced expression." There are two changes here: 1. Don't put a newline after the arrow. 2. When the only argument to a function is a lambda, don't put a newline after the open-paren of the function. I think this newline was going in because a lambda is a single expression that is longer than (the remainder of) a line. But generally, it's prettier to break inside the lambda.
google#19 The style guide says: "A line is never broken adjacent to the arrow in a lambda, except that a break may come immediately after the arrow if the body of the lambda consists of a single unbraced expression." There are two changes here: 1. Don't put a newline after the arrow. 2. When the only argument to a function is a lambda, don't put a newline after the open-paren of the function. I think this newline was going in because a lambda is a single expression that is longer than (the remainder of) a line. But generally, it's prettier to break inside the lambda.
google#19 The style guide says: "A line is never broken adjacent to the arrow in a lambda, except that a break may come immediately after the arrow if the body of the lambda consists of a single unbraced expression." There are two changes here: 1. Don't put a newline after the arrow. 2. When the only argument to a function is a lambda, don't put a newline after the open-paren of the function. I think this newline was going in because a lambda is a single expression that is longer than (the remainder of) a line. But generally, it's prettier to break inside the lambda.
+1 Formatting of lambdas in the current version (1.3) is quite annoying (gets unreadable quite easily). assertThat(response.getBody())
.satisfies(
body -> {
assertThat(body.data())
.hasValueSatisfying(
place -> {
assertThat(place.getId()).isEqualTo(253955);
});
}); |
+1 too many break line |
Not a huge fan of "+1" comments, but I'll commit the faux pax anyway to note that I'm surprised the "new line solely for lambda variable declaration" is not seen as a bigger deal/fixed by now. Curious why this is still unresolved? E.g. it is technically challenging? Or do the contributors think the current behavior is actually preferred? If so, maybe just decide so and close the issue? |
Hello, is there any update on this issue? |
Does anyone have any updates on this bug? |
@jkaldon @dmvk @stephenh @wangxufire Examples how it handles lambdas currently: Note that it is not perfect as mentioned by the original author at |
@cushon @ronshapiro I am posting to beg for the elimination of mandatory line breaks before the lambda parameter and arrow token. This is needed to avoid unreadable code. Please also see this explanation. |
I have set up indentation as the above in my checkstyle configuration. The issue I have with Google Java format is that it auto-indents lambda expressions by 4 spaces on the next line thus breaking the above rule. My source code is hosted at https://github.com/Fernal73/LearnJava and the project you'd want to look at is Switch under the Switch directory. The checkstyle rules are listed in csrules.xml under the root directory. Run the build.xml under the root directory to generate the classpath for Google java format and then either run the format and checkstyle scripts with the project name as parameter or run the build.xml under the Project Switch. The ant tasks are checkstyle and format respectively. The default build runs the formatter and then checkstyle. |
Checkstyle indentation rule. |
For some reason, the xml snippet is not accepted by this comment box. Kindly checkout the Module Indentation in csrules.xml in root directory. |
Would you prefer I list the above as a new issue? |
I'm wondering if this is a Google java format error. Line wrapping from the formatter is always four spaces. If I set the forceStrictCindition property in the checkstyle to true, then it flags every other line wap indentation as well as formatted by Google Java Formatter. One workaround would be to set that value to 4. I'm not happy with that. The question is why is the strict check enforced for lambda expressions by Checkstyle when it's set to false? |
@Fernal73 Using When running |
Thanks, Christian.
Your suggestion makes eminent sense especially about the indentation rule.
I use Checkstyle and PMD for more than just 'style', though.
Regards,
Linus.
|
Then disable the style-related checks in Checkstyle to prevent any "conflict" between the two tools, and only keep the non-style-related checks that you know won't conflict. |
I understand that Google-java-format is not configurable. Deliberately so.
However, I was wondering if it was possible for it to not disturb existing
formatting if it found it well within certain guidelines.
That's probably out of the question, though.
|
Yes, it does appear that the lambda not being complied with is a well-known
bug with checkstyle. I've disabled the indentation module in checkstyle and
everything's fine once more!
Thanks
|
|
Thanks. I'm aware of that.
In other words, if developers wish to tweak the Google Java Formatter, they
would be better off using other formatters that allow them to configure the
default Google style.
That's fair enough.
|
You can check out this clang-format file and let me know if it matches the
Google Java Formatter.
It's modified since the default config.
Regards,
Linus.
|
You might want to change the property AlignAfterOpenBracket to the original
Align.
|
Hi @Fernal73 - which clang-format file are you referring to? :) |
This one:
```
Language: Java
# BasedOnStyle: Google
AccessModifierOffset: -1
AlignAfterOpenBracket: DontAlign
AlignConsecutiveAssignments: false
AlignConsecutiveDeclarations: false
AlignEscapedNewlines: Left
AlignOperands: true
AlignTrailingComments: true
AllowAllParametersOfDeclarationOnNextLine: true
AllowShortBlocksOnASingleLine: false
AllowShortCaseLabelsOnASingleLine: false
AllowShortFunctionsOnASingleLine: None
AllowShortIfStatementsOnASingleLine: false
AllowShortLoopsOnASingleLine: false
AlwaysBreakAfterDefinitionReturnType: None
AlwaysBreakAfterReturnType: None
AlwaysBreakBeforeMultilineStrings: true
AlwaysBreakTemplateDeclarations: Yes
BinPackArguments: true
BinPackParameters: true
BraceWrapping:
AfterClass: false
AfterControlStatement: false
AfterEnum: false
AfterFunction: false
AfterNamespace: false
AfterObjCDeclaration: false
AfterStruct: false
AfterUnion: false
AfterExternBlock: false
BeforeCatch: false
BeforeElse: false
IndentBraces: false
SplitEmptyFunction: true
SplitEmptyRecord: true
SplitEmptyNamespace: true
BreakBeforeBinaryOperators: NonAssignment
BreakBeforeBraces: Attach
BreakBeforeInheritanceComma: false
BreakInheritanceList: BeforeColon
BreakBeforeTernaryOperators: true
BreakConstructorInitializersBeforeComma: false
BreakConstructorInitializers: BeforeColon
BreakAfterJavaFieldAnnotations: false
BreakStringLiterals: true
ColumnLimit: 80
CommentPragmas: '^ IWYU pragma:'
CompactNamespaces: false
ConstructorInitializerAllOnOneLineOrOnePerLine: true
ConstructorInitializerIndentWidth: 4
ContinuationIndentWidth: 4
Cpp11BracedListStyle: true
DerivePointerAlignment: true
DisableFormat: false
ExperimentalAutoDetectBinPacking: false
FixNamespaceComments: true
ForEachMacros:
- foreach
- Q_FOREACH
- BOOST_FOREACH
IncludeBlocks: Preserve
IncludeCategories:
- Regex: '^<ext/.*\.h>'
Priority: 2
- Regex: '^<.*\.h>'
Priority: 1
- Regex: '^<.*'
Priority: 2
- Regex: '.*'
Priority: 3
IncludeIsMainRegex: '([-_](test|unittest))?$'
IndentCaseLabels: true
IndentPPDirectives: None
IndentWidth: 2
IndentWrappedFunctionNames: false
JavaScriptQuotes: Leave
JavaScriptWrapImports: true
KeepEmptyLinesAtTheStartOfBlocks: false
MacroBlockBegin: ''
MacroBlockEnd: ''
MaxEmptyLinesToKeep: 1
NamespaceIndentation: None
ObjCBinPackProtocolList: Never
ObjCBlockIndentWidth: 2
ObjCSpaceAfterProperty: false
ObjCSpaceBeforeProtocolList: true
PenaltyBreakAssignment: 2
PenaltyBreakBeforeFirstCallParameter: 1
PenaltyBreakComment: 300
PenaltyBreakFirstLessLess: 120
PenaltyBreakString: 1000
PenaltyBreakTemplateDeclaration: 10
PenaltyExcessCharacter: 1000000
PenaltyReturnTypeOnItsOwnLine: 200
PointerAlignment: Left
RawStringFormats:
- Language: Cpp
Delimiters:
- cc
- CC
- cpp
- Cpp
- CPP
- 'c++'
- 'C++'
CanonicalDelimiter: ''
BasedOnStyle: google
- Language: TextProto
Delimiters:
- pb
- PB
- proto
- PROTO
EnclosingFunctions:
- EqualsProto
- EquivToProto
- PARSE_PARTIAL_TEXT_PROTO
- PARSE_TEST_PROTO
- PARSE_TEXT_PROTO
- ParseTextOrDie
- ParseTextProtoOrDie
CanonicalDelimiter: ''
BasedOnStyle: google
ReflowComments: true
SortIncludes: true
SortUsingDeclarations: true
SpaceAfterCStyleCast: false
SpaceAfterTemplateKeyword: true
SpaceBeforeAssignmentOperators: true
SpaceBeforeCpp11BracedList: false
SpaceBeforeCtorInitializerColon: true
SpaceBeforeInheritanceColon: true
SpaceBeforeParens: ControlStatements
SpaceBeforeRangeBasedForLoopColon: true
SpaceInEmptyParentheses: false
SpacesBeforeTrailingComments: 2
SpacesInAngles: false
SpacesInContainerLiterals: true
SpacesInCStyleCastParentheses: false
SpacesInParentheses: false
SpacesInSquareBrackets: false
Standard: Auto
StatementMacros:
- Q_UNUSED
- QT_REQUIRE_VERSION
TabWidth: 2
UseTab: Never
```
|
You might want to set AllowShortLambdasOnASingleLine to false if you're
able to access version 9 of clang-format.
|
Any progress on this? |
What do you mean? No, I don't have access to clang-9 on termux, as yet. The only workaround for now is to manually format the code as expected and wrap it in // clang-format off and // clang-format on markers. That works if you're using clang-format. It will not prevent Google-Java-format from reformatting your code if it deems it necessary. |
My usecase is to use Google-Java-format and clang-format in that order. If I don't like what clang-format does, I comment-block that section. Google-Java-format does a slightly better job, overall, but then I haven't access to clang-format 9. |
Since almost a year passed after the last comment and there was a release of GJF recently (so at least the project isn't dead): Is there any progress on improving the formatting of lambdas by GJF? |
I would like to echo the above comment I really don't feel like adding a line break or new line solely for the purpose of using a language feature is a good workaround. |
If you want to have a solution now: Get rid of GJF and use spotless to run the Eclipse JDT formatter. |
Wow. I apologize for not having given any update on this issue in so long (and while so much discussion happened). As I suppose is obvious, we haven't been paying attention in an "organized/committed" way but have just watched for low-hanging fruit. This one seems to be... high-hanging fruit:
|
Note that this behavior was the motivation of the fork "Palantir Java Format". However, they seem not to have any capacity to suport Java 21 (see palantir/palantir-java-format#952). |
are the extra newlines intentional?
before:
or even
after:
The text was updated successfully, but these errors were encountered: