Auto-formatting tweaks #47299
Auto-formatting tweaks #47299
Conversation
|
Pinging @elastic/es-core-infra |
| @@ -68,6 +68,7 @@ | |||
|
|
|||
| static final List<String> DEFAULT_PROTOCOLS = List.of("TLSv1.3", "TLSv1.2", "TLSv1.1"); | |||
|
|
|||
| // @formatter:off | |||
There was a problem hiding this comment.
This seems like an important tool we should document in our CONTRIBUTING.md? (separate from this PR)
There was a problem hiding this comment.
I can add it to CONTRIBUTING.md, I should have adding it in the last PR to be honest.
| // part of the other range must touch this range | ||
| // this: |---------------| | ||
| // other: |------| | ||
| /* |
There was a problem hiding this comment.
Why is this converted to a multiline comment? Is there some problem with using several single line comments with the formatter?
There was a problem hiding this comment.
I observed the formatted mangling these diagrams, hence the change. You can ask the formatter to process block and javadoc comments, but I decided that wasn't worth it.
| + "\"collate_match\":true}]" | ||
| + "}]" | ||
| + "}", xContent.utf8ToString()); | ||
| // @formatter:off |
There was a problem hiding this comment.
We have lines like this in a lot of tests. Is there anything we can do to improve this without resorting to manually turning off the formatter?
There was a problem hiding this comment.
There are options, depending on the code in question.
JSON snippets
For this example, we could include whitespace:
String suggestionString = "{"
+ "\"unknownType#suggestionName\": ["
+ " {"
+ " \"text\": \"entryText\","
+ " \"offset\": 42,"
+ " \"length\": 313,"
+ " \"options\": [{"
+ " \"text\": \"someText\","
+ " \"highlighted\": \"somethingHighlighted\","
+ " \"score\": 1.3,"
+ " \"collate_match\": true"
+ " }]"
+ " }]"
+ "}";With Java 13, we could use text blocks:
String suggestionString = """
{
"unknownType#suggestionName": [
{
"text": "entryText",
"offset": 42,
"length": 313,
"options": [{
"text": "someText",
"highlighted": "somethingHighlighted",
"score": 1.3,
"collate_match": true
}]
}]
}
""";
We could also make these snippets more readable by using an ObjectMapper with some lenient settings, e.g:
public static String asJson(final String rawJson) throws IOException {
ObjectMapper mapper = new ObjectMapper();
mapper.configure(JsonParser.Feature.ALLOW_UNQUOTED_FIELD_NAMES, true);
mapper.configure(JsonParser.Feature.ALLOW_SINGLE_QUOTES, true);
final JsonNode jsonNode = mapper.readTree(rawJson);
return mapper.writeValueAsString(jsonNode);
}Then we can write the JSON like so:
String suggestionString = asJson("{"
+ "'unknownType#suggestionName': ["
+ " {"
+ " text: 'entryText',"
+ " offset: 42,"
+ " length: 313,"
+ " options: [{"
+ " text: 'someText',"
+ " highlighted: 'somethingHighlighted',"
+ " score: 1.3,"
+ " collate_match: true"
+ " }]"
+ " }]"
+ "}");Depend how wild we're feeling, we could define these snippets in YAML, and convert to JSON, e.g.:
String suggestionYaml = yamlToJson(
"unknownType#suggestionName:\n"
+ " - text: entryText\n"
+ " offset: 42\n"
+ " length: 313\n"
+ " options:\n"
+ " - text: someText\n"
+ " highlighted: somethingHighlighted\n"
+ " score: 1.3\n"
+ " collate_match: true\n");Though I expect that'll look better with text blocks, once they're available:
String suggestionYaml = yamlToJson("""
unknownType#suggestionName:
- text: entryText
offset: 42
length: 313
options:
- text: someText
highlighted: somethingHighlighted
score: 1.3
collate_match: true
""");
Aligned code
The changes to SslConfigurationLoader.java are a case where strings and comments are arranged in meaningful rows. I think this is a case where just exempting the code is the best approach.
On that subject, however, the Painless module is interesting because it has code that has vertical alignment. For example:
I can't decide whether we should turn off the formatter for these. What do you think?
Also, Spotless doesn't appear to have a way to exempt files from formatting, so it formats all the source that ANTLR generates too. Can't do much about that.
There was a problem hiding this comment.
@rjernst do you want me to do anything here? I could go looking for more code whose format we want to preserve?
There was a problem hiding this comment.
Long term I think we will want to use text blocks, but it will be a long time before we can do that. I wouldn't change to yaml, since we are testing json. Ideally these would actually be converted to xcontentbuilder, since that has earlier error checking (eg invalid json would get caught when running the code, rather than in the request parsing the json). My concern here is starting this precedent of arbitrarily turning off the formatter. Using column aligned strings seems like the best option for now, but we should probably change those in separate PRs from any enabling auto formatting, so that the formatting PRs can remain completely automatic and mechanical in nature, thus simplifying reviewing.
For cases where the formatter was disabled around JSON snippets, embed more whitespace within the strings to as to format the JSON in a way that the formatter won't break. Also, change the formatter's enable/disables tags to that the documentation comments i.e "// tag::NAME" and "// end::NAME" still work. This means that such snippets won't be automatically formatted, but since the formatter can't be configured different for specific regions in a file, it's the best we can do.
Make a number of code changes so that when the codebase is automatically
formatted, the code doens't end up looking mangled. Mostly this is:
- Moving comments
- Joining multiline strings that fit on one line
- Formatting JSON so that indentation is inside the strings
- This includes stripping out additional whitespace in some cases,
where the test does an exact comparison with JSON from the API
Also reformat XML documents in the SAML tests, using a formatter that
can handle named parameters. This significantly improves legibility.
|
@rjernst I've reworked the code where I'd disabled the formatter, and then carried on to audit the repository post-formatting. I've made a number of further changes so that some code doesn't appear mangled after formatting. Mostly this is:
I also reformatted the XML documents in the SAML tests, using a formatter that There are still some other files that could use some love, for example the geo parser tests. They're building up documents via XContent, and since the formatter sees these as a long sequence of method calls, that exactly how they're formatted. I think we're better off just using JSON here - it'd be significantly terser, and would retain indentation. |
|
@elasticmachine test this please |
|
Thanks for working through the formatter disabling issues. Something I've realized upon looking back at this is the enormous size in conjunction with several changes being made to the format. Would it be possible to separate out each of the listed tweaks in the description to their own PRs? If a PR is 1k lines, it is generally difficult to adequately review, and this PR is almost 4k edits. I understand these tweaks will necessarily touch a lot of code and be large, but it is much easier to scroll through a large PR when there is a singular purpose, and thus one typical pattern to visually look for. |
|
@rjernst sure, the PR wasn't really supposed to be this size. I'll split up the changes and list them here for posterity.
|
Follow-up to #46745. Tweak the Eclipse formatter config. It's hard to see what different is from the XML, so here's a summary:
Also tweak some source files so that they aren't adversely affected when we finally format the source.