-
Notifications
You must be signed in to change notification settings - Fork 499
Wrapping and braces style #6429
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
base: main
Are you sure you want to change the base?
Conversation
rewrite-java/src/main/java/org/openrewrite/java/format/WrappingAndBracesVisitor.java
Show resolved
Hide resolved
…gAndBracesVisitor.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
This is a really impressive piece of work! I particularly like the architectural approach of routing formatting logic through The addition of comprehensive Performance ConcernI do have a concern about the performance characteristics of the current implementation. The Cursor printCursor = cursor;
if (!(cursor.getValue() instanceof JavaSourceFile)) {
printCursor = cursor.dropParentUntil(c -> c instanceof JavaSourceFile);
}
javaPrinter.visit(printCursor.getValue(), printLine, printCursor.getParent());In
With default IntelliJ settings where several Potential SolutionsA few ideas that might help: 1. Print from a local anchor pointInstead of printing from the file root, print from a nearby anchor (e.g., the enclosing statement or method). Column position within a statement is what matters for most wrapping decisions, and statements are typically short enough that printing them is cheap. 2. Track column position incrementallySimilar to how
This would give O(n) traversal instead of O(n^2). 3. Hybrid approachKeep the centralized
This would preserve the PR's architectural elegance while avoiding the performance pitfall. Overall this is heading in a great direction - I just want to flag the performance aspect before it becomes an issue on larger codebases. |
|
Thanks @knutwannheden for this! I am looking at the TabsAndIndentsVisitor right now to see how I could reuse patterns from over there to not have to print entire file all the time. The other approach of anchor-printing also popped up in my mind. Printing as of the first element that starts on a new line would already prevent the entire file of being printed all the time. I also considered just adding the new line only and leaving indentation to the TabsAndIndentsVisitor, but that will give issues later as the WrapIfLong needs to know the indent of other container elements before it can decide if it is long or now. |
rewrite-groovy/src/main/java/org/openrewrite/groovy/format/AutoFormatVisitor.java
Outdated
Show resolved
Hide resolved
rewrite-java-test/src/test/java/org/openrewrite/java/format/WrappingAndBracesTest.java
Outdated
Show resolved
Hide resolved
rewrite-java-test/src/test/java/org/openrewrite/java/format/WrappingAndBracesTest.java
Show resolved
Hide resolved
rewrite-java-test/src/test/java/org/openrewrite/java/format/WrappingAndBracesTest.java
Show resolved
Hide resolved
rewrite-java-test/src/test/java/org/openrewrite/java/format/WrappingAndBracesTest.java
Show resolved
Hide resolved
|
There's one thread still unresolved it seems, but other than that what I've seen looks good to me; thanks for the massive effort here! |
timtebeek
left a comment
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.
Approved from my side already. Might need a coordinated merge & release given the packages changed.
| @Value | ||
| @With | ||
| public static class MethodDeclarationParameters { | ||
| Boolean alignWhenMultiple; | ||
| } | ||
|
|
||
| @Value | ||
| @With | ||
| public static class RecordComponents { | ||
| Boolean alignWhenMultiple; | ||
| } |
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 these classes are in a style package, I think we'd need a coordinated release right? Given that if ("tree".equals(part) || "marker".equals(part) || "style".equals(part)) we spotted earlier today
|
We should be good i guess. The evaluate method handles the package changes in cli and it also handled it for previous changes. But better safe than sorry 😊 |
# Conflicts: # rewrite-java/src/test/java/org/openrewrite/java/format/WrapMethodChainsTest.java
knutwannheden
left a comment
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 haven't reviewed the visitor logic. I think the best would be to run the formatter over some repos and see if we detect any regressions (e.g. by comparing patches) or if we find some more cases we can fix.
| new WrappingAndBracesStyle.Annotations(DoNotWrap), | ||
| new WrappingAndBracesStyle.Annotations(DoNotWrap), | ||
| new WrappingAndBracesStyle.Annotations(DoNotWrap) | ||
| new WrappingAndBracesStyle.KeepWhenFormatting(true, true, true, true, false, false, false, false), |
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.
Please verify:
| new WrappingAndBracesStyle.KeepWhenFormatting(true, true, true, true, false, false, false, false), | |
| new WrappingAndBracesStyle.KeepWhenFormatting(true, true, true, false, false, false, false, false), |
| new WrappingAndBracesStyle.MethodParentheses(false, false), | ||
| new WrappingAndBracesStyle.ChainedMethodCalls(LineWrapSetting.DoNotWrap, false, false, emptyList(), false, false), | ||
| new WrappingAndBracesStyle.IfStatement(WrappingAndBracesStyle.ForceBraces.DoNotForce, false, true), | ||
| new WrappingAndBracesStyle.ForStatement(LineWrapSetting.DoNotWrap, false, false, false, WrappingAndBracesStyle.ForceBraces.DoNotForce), |
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.
Please verify:
| new WrappingAndBracesStyle.ForStatement(LineWrapSetting.DoNotWrap, false, false, false, WrappingAndBracesStyle.ForceBraces.DoNotForce), | |
| new WrappingAndBracesStyle.ForStatement(LineWrapSetting.DoNotWrap, true, false, false, WrappingAndBracesStyle.ForceBraces.DoNotForce), |
| @Option(displayName = "Remove custom line breaks", | ||
| description = "Do you want to remove custom line breaks? (default false)", | ||
| required = false) | ||
| @Nullable | ||
| Boolean removeCustomLineBreaks; |
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 am glad to see this go 👍
|
|
||
| @ToBeRemoved(after = "30-11-2025", reason = "Replace me with org.openrewrite.style.StyleHelper.addStyleMarker now available in parent runtime") | ||
| @ToBeRemoved(after = "30-01-2025", reason = "Replace me with org.openrewrite.style.StyleHelper.addStyleMarker now available in parent runtime") | ||
| private static <T extends SourceFile> T addStyleMarker(T t, List<NamedStyles> styles) { | ||
| if (!styles.isEmpty()) { |
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.
Why did this date go back in time? Probably safe to remove this now
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 missed the year. I would not like to introduce the risk of NoSuchMethodErrors in this PR.
Subsequent PR will follow that will remove this one. Updated the dates of these annotations so we can remove all of them at once.
| int endColumn; | ||
| int maxColumn; |
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.
What is the difference between "end column" and "max column" ? I would have assumed these were the same... for that matter why do we need this as a separate concept from Span ?
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.
There are 2 question in this comment afaics.
-
Difference between endColumn and maxColumn:
Some elements will span multiple lines. The endColumn is the column on which the entire lst element printed is ending. The maxColumn is the width of the printed block. Whichever line is the longest will have its length in the maxColumn. -
Difference between Span and ColSpan:
For Span (which has line information), we have to print the file as of the SourceFile's beginning. For Colspan, we only have to print the lst element which starts on a new line (as we do not care about the lines).
During calculations in WrappingAndBraces / Indents ... we only need column information. This saves a lot of printing during the calculation and then also reduces the runtime of these visitors.
What's changed?
Introduced all properties in the WrappingAndBraces style and partially support their wrapping.
Added additional support for a bunch of properties.
Refactored the existing support in the WrappingAndBracesVisitor to all do their logic through the same "visitSpace" concept rather than dedicated visitors for every type of J element.
What's your motivation?
Not all of the style properties are supported yet, but this will allow us to clean up the
evaluate()pattern sooner for all properties as the parent loading will have all properties for all lst elements when we've add support for these wrappings. The unsupported ones are also ones that are less likely to be used in initial usages. This is already a big improvement compared to the before-state.There is still a small need for the TabsAndIndentsVisitor currently, but I feel we can get rid of this one in a next iteration as only very little amount of indents are now not immediately correct after WrappingAndBraces -> planning to add the little missing support to that one and then deprecate TabsAndIndents to save a traversal (actually, make up for the one that got introduced by the MergeSpacesVisitor)
Checklist