Consistent formatting of multiline strings #23958

losipiuk · 2024-10-29T13:56:03Z

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

losipiuk · 2024-10-29T15:50:54Z

Not sure if that is really nicer? But @martint you requested changes like this on on PR so thought i would make sense to just use common formatting every where.

core/trino-main/src/test/java/io/trino/sql/planner/TestOrderBy.java

martint · 2024-10-29T16:02:02Z

core/trino-main/src/test/java/io/trino/sql/planner/TestTableFunctionInvocation.java

@@ -238,31 +238,35 @@ public void testRemoveRedundantTableFunction()
        assertPlan("SELECT * FROM TABLE(mock.system.pass_through_function(input => TABLE(SELECT 1, true WHERE false) t(a, b) PRUNE WHEN EMPTY))",
                output(values(ImmutableList.of("x", "a", "b"))));

-        assertPlan("""
+        assertPlan(
+                        """


Indentation seems wrong here and in the ones below, too.

Yeah - but to be honest I am not sure how should it be.
Also no matter what I do automatic reformat in intellij breaks it anyway to:

assertPlan( """ SELECT * FROM TABLE(mock.system.two_table_arguments_function( input1 => TABLE(SELECT 1, true WHERE false) t1(a, b) PRUNE WHEN EMPTY, input2 => TABLE(SELECT 2, false) t2(c, d) KEEP WHEN EMPTY)) """,

This PR brings back the discussion about automatic code reformatting. IMO it would be great to have. One less thing to think of - even if not optimal from time to time.

See #21815 (comment)

Yeah - this is a mess.
I really think we should commit on using Intellij formatter and plug it in as a headless verifier in CI pipeline to verify if checked in code matches the formatting

core/trino-main/src/test/java/io/trino/type/TestArrayOperators.java

core/trino-parser/src/test/java/io/trino/sql/TestSqlFormatter.java

core/trino-parser/src/test/java/io/trino/sql/parser/TestSqlParser.java

martint · 2024-10-29T16:09:12Z

Not sure if that is really nicer?

It's needed to make the auto-formatter in the IDE not mess things up.

testing/trino-testing/src/main/java/io/trino/testing/AbstractTestAggregations.java

wendigo · 2024-10-30T12:20:00Z

testing/trino-testing/src/main/java/io/trino/testing/AbstractTestAggregations.java

+                        "   (2, 2)," +
+                        "   (2, 2)," +
+                        "   (3, 3)" +
+                        ") t(x, y)",
                "VALUES (1.0, 1.0, 4)");

        assertQuery(
                "SELECT corr(DISTINCT x, y), corr(DISTINCT y, x), count(DISTINCT x) FROM " +


make it a multiline string instead

testing/trino-testing/src/main/java/io/trino/testing/AbstractTestAggregations.java

plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveS3AndGlueMetastoreTest.java

plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/TestPinotConnectorSmokeTest.java

...rino-product-tests/src/main/java/io/trino/tests/product/hudi/TestHudiSparkCompatibility.java

testing/trino-testing/src/main/java/io/trino/testing/AbstractTestAggregations.java

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java

martint · 2024-10-30T16:23:39Z

client/trino-cli/src/main/java/io/trino/cli/QueryPreprocessor.java

@@ -175,6 +176,7 @@ private static String preprocessQueryInternal(Optional<String> catalog, Optional
                    message = "\n===\n" + message + "\n===";
                }
                catch (Exception ignored) {
+                    // ignore


All these "// ignore" comments are noisy and unnecessary. It is clear from the exception name and the empty body that the exception is being ignored.

I wanted {} - but checkStyle complains.
Will do

{ }

Plus some other formatting improvements

mosabua · 2024-10-31T23:49:38Z

I recently played with the spotless plugin .. its MUCH nicer than checkstyle .. we should migrate to it .. upon request from @losipiuk I added this to the agenda for next maintainer meeting

cla-bot bot added the cla-signed label Oct 29, 2024

github-actions bot added docs iceberg Iceberg connector delta-lake Delta Lake connector hive Hive connector bigquery BigQuery connector labels Oct 29, 2024

losipiuk marked this pull request as ready for review October 29, 2024 15:49

losipiuk requested a review from martint October 29, 2024 15:49

losipiuk requested a review from wendigo October 29, 2024 15:51

martint reviewed Oct 29, 2024

View reviewed changes

losipiuk force-pushed the lukaszos/consistent-formatting-of-multiline-strings-f88f9b branch from e1841ec to 332a755 Compare October 30, 2024 12:13