-
Notifications
You must be signed in to change notification settings - Fork 68
Format text blocks + Intellij reflows strings #1419
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: develop
Are you sure you want to change the base?
Conversation
Generate changelog in
|
|
||
@Override | ||
protected float computeWidth() { | ||
if (token.getTok().getOriginalText().startsWith(StringWrapper.TEXT_BLOCK_DELIMITER)) { |
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.
before - when the textBlock+methods would exceed 120 characters (maxWidthLimit), the block would look like this:
"""
hello %s this exceeds 120 characters nasddnsadalsdaslda
"""
.formatted("world");
now:
"""
hello %s this exceeds 120 characters nasddnsadalsdaslda
""".formatted("world");
if (!StringWrapper.linesNeedWrapping(options.maxLineLength(), formattedText, characterRanges)) { | ||
return writeFormatReplacements(replacements); | ||
} | ||
String wrappedFormattedText = StringWrapper.wrap(options.maxLineLength(), formattedText, formatter); |
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.
same behaviour now as with the gradle formatter task:
Line 72 in 1cc1160
return memoizedFormatter.get()::formatSourceReflowStringsAndFixImports; |
The alternative would have been to directly call the same method in the Intellij plugin, however I think this would be a bit more optimal - because when there is no need to call the wrapper, we can simply return just the replacement ranges. Worst case happens here where we have to send back the full document range to be reformatted.
throws FormatterException { | ||
JCTree.JCCompilationUnit unit = parse(input, /* allowStringFolding= */ false); | ||
String separator = Newlines.guessLineSeparator(input); | ||
return new Reflower(columnLimit, input).getReflowReplacements(); |
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.
tried to keep this in line with pjf logic - refactored to be similar to pjf - as it easier to compare diffs.
String prefix = (lineStartPosition + startColumn + 4 > startPosition ? "" : " ".repeat(4)) | ||
+ " ".repeat(startColumn); | ||
|
||
StringBuilder output = new StringBuilder(initialLines.get(0)); |
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.
diverges from gjf to ensure we indent text blocks
* column limit or contain text blocks. Used by the Intellij plugin to check if we need to run StringWrapper.wrap. | ||
* Keep this method and {@code needWrapping} in line. | ||
* */ | ||
public static boolean linesNeedWrapping(int columnLimit, String input, RangeSet<Integer> initialRangesToChange) { |
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.
pjf specific - we need to figure out if we need to wrap the current input in the rangeSet initialRangesToChange
✅ Successfully generated changelog entry!What happened?Your changelog entries have been stored in the database as part of our migration to ChangelogV3. Need to regenerate?Simply interact with the changelog bot comment again to regenerate these entries. |
""" + """ | ||
bar | ||
"""; | ||
String u = stringVariableOne + """ |
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.
unfortunately string concatenation is not great with text blocks and I am not sure if I can change it.
after running the AST formatter I am getting (The AST cannot change the indentation of the text block node):
String u = stringVariableOne + """
...
""" + stringVariableTwo + """
...
""";
and then after I try to re-indent the text blocks and given that I am trying to indent based on the previous line, I am getting the block below.
For these kind of text block operations, people should try to keep the textBlocks indented at the same line with the rest of the operands and then the block will be properly indented
Depends on palantir/gradle-baseline#3276. Adding |
int lineEnd = end; | ||
while (Newlines.hasNewlineAt(input, lineEnd) == -1) { | ||
lineEnd++; | ||
private void indentTextBlocks(TreeRangeMap<Integer, String> replacements, List<Tree> textBlocks) { |
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 general the changes in this PR look much better! I ran it on gradle-guide here and most of the changes are an improvement imo.
The only weirdness (on this repo at least) is around multiple text block arguments to the same method, example:
refactorFromTo("""
someCode
""", """
otherCode
""");
I think the second text block is getting indented out of line with the first because the first ending """
has been indented?
You can take this to it's logical extreme:
void lotsOfStringParams(String a, String b, String c, String d, String e, String f, String g) {}
void test() {
lotsOfStringParams("""
aaa
""", """
bbb
""", """
ccc
""", """
ddd
""", """
eee
""", """
fff
""", """
ggg
""");
}
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.
Ah, this might be the same as a concatenation issue?
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.
If you force it to put each arg on a new line by making one arg very long, it kinda looks better but stops the """, """
pattern:
void lotsOfStringParams(String a, String aa, String b, String c) {}
void test() {
lotsOfStringParams(
"""
aaa
""",
"sdklfjslfkjsadlkgjsdklfjsadlkfjsaklfjaskdsfkljsaklfjsadflkjsdafkljasdfklsjfklsajflkasjfsfjksflfj",
"""
bbb
""",
"""
ccc
""");
}
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.
re: last example with the one long arg
I believe this has to do with how parameters for a method are going to be structured if the maxLineColumn would exceed - same behaviour if we had long variable names/long annotated variables.
void test(
String aaaaaaaaaa,
@NotNull @Variable Ssdklfjslfkjsdasdadasasdsadsadasdasadadasddadsasdsadadasdadadasdas variable,
String bbbbbb) {
lotsOfStringParams(
aaaaaaaaaa,
sdklfjslfkjsdasdadasasdsadsadasdasadadasddadsasdsadadasdadadasdasadadasdasfasfasfasfas,
bbbbbb);
}
Before this PR
After this PR
Fixes: #930
Fixes: #1331
"""
on the previous line, the text block gets reindented.Example of a re-format: https://pl.ntr/2x2
==COMMIT_MSG==
Format text blocks + Intellij reflows strings
==COMMIT_MSG==
Possible downsides?