-
Notifications
You must be signed in to change notification settings - Fork 83
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
Changes from all commits
6743e8e
95c4ee5
fd96d13
23c91f9
96d304a
aa324c8
11d0c00
043db14
8063b4d
19e6298
2730c64
946543a
c618730
87f1e1d
d10b6ce
fe517fb
ab9282c
8169884
1815e21
98f5f53
5439660
d52de5e
f9bc8dd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,8 @@ | |
| /node_modules | ||
| /.pnp | ||
| .pnp.js | ||
| yarn.lock | ||
| package-lock.json | ||
|
|
||
| # testing | ||
| /coverage | ||
|
|
||
This file was deleted.
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -18,17 +18,18 @@ | |||
| import com.fasterxml.jackson.databind.ObjectMapper; | ||||
| import com.fasterxml.jackson.databind.json.JsonMapper; | ||||
| import com.fasterxml.jackson.datatype.guava.GuavaModule; | ||||
| import com.google.common.collect.ImmutableList; | ||||
| import com.google.common.collect.Range; | ||||
| import com.google.common.collect.RangeSet; | ||||
| import com.google.common.collect.TreeRangeSet; | ||||
| import com.palantir.javaformat.Utils; | ||||
| import java.io.UncheckedIOException; | ||||
| import java.util.List; | ||||
| import java.util.Set; | ||||
| import java.util.concurrent.Callable; | ||||
|
|
||||
| /** Encapsulates information about a file to be formatted, including which parts of the file to format. */ | ||||
| class FormatFileCallable implements Callable<String> { | ||||
| private final ObjectMapper MAPPER = | ||||
| private static final ObjectMapper MAPPER = | ||||
| JsonMapper.builder().addModule(new GuavaModule()).build(); | ||||
|
|
||||
| private final String input; | ||||
|
|
@@ -49,14 +50,29 @@ public String call() throws FormatterException { | |||
|
|
||||
| Formatter formatter = Formatter.createFormatter(options); | ||||
| if (parameters.outputReplacements()) { | ||||
| return formatReplacements(formatter); | ||||
| RangeSet<Integer> characterRanges = characterRanges(input); | ||||
| Set<Range<Integer>> rangesToChange = characterRanges.asRanges(); | ||||
| List<Replacement> replacements = formatter.getFormatReplacements(input, rangesToChange); | ||||
| if (parameters.reflowLongStrings()) { | ||||
| String formattedText = Utils.applyReplacements(input, replacements); | ||||
| // avoid trying to wrap the input unless the lines needing wrapping are part of the "characterRanges" | ||||
| if (!StringWrapper.linesNeedWrapping(options.maxLineLength(), formattedText, characterRanges)) { | ||||
| return writeFormatReplacements(replacements); | ||||
| } | ||||
| String wrappedFormattedText = StringWrapper.wrap(options.maxLineLength(), formattedText, formatter); | ||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
The alternative would have been to directly call the same method in the Intellij plugin, however I think this would be a bit more optimized - 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 - it happens for any reflow or text block reformatting
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Am I reading around this code correctly that this
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, no, I think this is always the case when running through the Intellij plugin (same bug that gjf has here as well issue 566 in gjf) When running the Intellij plugin we will always try to run the getFormatReplacements which doesn't include the String.wrapper call (I guess this is because the String.wrapper will always reformat the whole file, whereas in the intellij plugin we might get a subset of lines from the initial file to be formatted). However, through cli, we are calling the other mehtod which always calls the StringWrapper. |
||||
| // if no wrapping change happened, return the initial replacements | ||||
| if (wrappedFormattedText.equals(formattedText)) { | ||||
| return writeFormatReplacements(replacements); | ||||
| } | ||||
| return writeFormatReplacements(List.of(Replacement.create(0, input.length(), wrappedFormattedText))); | ||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. worst case scenario here - when string reflows happens/text blocks are indented, then we will return the full range of the file to be replaced. This could be optimized - I think there is a way to check where the text was reformatted and return only those ranges. I can add this as a FLUP to the PR. |
||||
| } else { | ||||
| return writeFormatReplacements(replacements); | ||||
| } | ||||
| } | ||||
| return formatFile(formatter); | ||||
| } | ||||
|
|
||||
| private String formatReplacements(Formatter formatter) throws FormatterException { | ||||
| ImmutableList<Replacement> replacements = | ||||
| formatter.getFormatReplacements(input, characterRanges(input).asRanges()); | ||||
| String writeFormatReplacements(List<Replacement> replacements) throws FormatterException { | ||||
| try { | ||||
| return MAPPER.writeValueAsString(replacements); | ||||
| } catch (JsonProcessingException e) { | ||||
|
|
||||
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:
now: