Replacing the linter

[Jolanrensen]

Fixes #364, the last stopper for updating to Kotlin 2.0.

• replaced kotlinter by jlleitschuh's ktlint plugin
• .editorconfig is used to set the rules of the linter and the code style
• Please use https://plugins.jetbrains.com/plugin/15057-ktlint from now on :)
• Individual cases can be @Suppress-ed easily with that plugin
• I mostly-automatically refactored the entire project, but I checked all changes
• I fixed some manual things: *-imports, names of consts, @file:ExcludeFromSources to prevent "empty" files in generated-sources
• Tests in samples/api are cleaned for now and ignored by ktlint on a per-file basis (@file:Suppress("ktlint"))
• See the issue for more information

[zaleslaw]

is it really linter setting? could we change it? I don't like these short forms without brackets, it looks ugly and make me slower

[Jolanrensen]

It is https://pinterest.github.io/ktlint/latest/rules/standard/#function-expression-body.
Because of the official style guide: https://kotlinlang.org/docs/coding-conventions.html#functions.
To me, block bodies with just one return statement take up unnecessary space and feel like java-boilerplate. We could change it, it's a matter of taste. But I think the most important thing is consistency; Mixing single-line block body's and expression bodies is messy.

[zaleslaw]

and again, here we don't need these brackets in the code

[Jolanrensen]

It's more readable for multiline if-statements: https://pinterest.github.io/ktlint/latest/rules/standard/#if-else-bracing and https://kotlinlang.org/docs/coding-conventions.html#control-flow-statements.
No brackets in if-statements are only allowed/clear when it fits on the same line.

[zaleslaw]

All inserts looks now to so cool as earlier, they should be single-rowd

[Jolanrensen]

@zaleslaw

It can be solved either like:

connection.createStatement()
    .execute("INSERT INTO TestTable1 (id, name, surname, age) VALUES (1, 'John', 'Crawford', 40)")
connection.createStatement()
    .execute("INSERT INTO TestTable1 (id, name, surname, age) VALUES (2, 'Alice', 'Smith', 25)")
connection.createStatement()
    .execute("INSERT INTO TestTable1 (id, name, surname, age) VALUES (3, 'Bob', 'Johnson', 47)")
connection.createStatement()
    .execute("INSERT INTO TestTable1 (id, name, surname, age) VALUES (4, 'Sam', NULL, 15)")

or by adding @Suppress("ktlint:standard:max-line-length") to the function declaration. What would you prefer? :)

[zaleslaw]

sorry, but it's fine with signatures, but not fine with the client code for passing parameters, it breaks the reading for me

[Jolanrensen]

Yes, this can indeed look better. How about:

dbType.convertSqlTypeToColumnSchemaValue(tableColumnMetadata) 
    ?: ColumnSchema.Value(makeCommonSqlToKTypeMapping(tableColumnMetadata))

or

dbType.convertSqlTypeToColumnSchemaValue(tableColumnMetadata) 
    ?: ColumnSchema.Value(
          makeCommonSqlToKTypeMapping(tableColumnMetadata),
    )

?

[zaleslaw]

what's with the formatting here?

[Jolanrensen]

It's a double indent

• something
  • like this

[zaleslaw]

@Jolanrensen you did a great job leading us forward to K2, but let's discuss that when you return and change if possible some rules

[Jolanrensen]

@Jolanrensen you did a great job leading us forward to K2, but let's discuss that when you return and change if possible some rules

Thanks for reviewing!

I think some of your suggestions are in conflict with the official Kotlin styleguide which are honored by Ktlint. We could deviate as I've done with a small number of rules in the .editorconfig file but we would need to be sure it'd be in the best interest of the library and our users.

For example, I've turned off the filename check, as we name our API files after the function-family it represents. I also turned off chain-method-continuation so DataFrame notations like:

df.update { .. }.where { .. }.with { .. }
  .update { .. }.with { .. }

are allowed and not replaced with:

df
  .update { .. }
  .where { .. }
  .with { .. }
  .update { .. }
  .with { .. }

[github-actions]

Generated sources will be updated after merging this PR.
Please inspect the changes in here.

[Jolanrensen]

As for the trailing-comma case, there are many viewpoints on what's best.
The Kotlin coding convention specifies many benefits:

It makes version-control diffs cleaner – as all the focus is on the changed value.

It makes it easy to add and reorder elements – there is no need to add or delete the comma if you manipulate elements.

It simplifies code generation, for example, for object initializers. The last element can also have a comma.

It even encourages it at declaration site (so defining classes, functions, etc.) and leaves it at our own discretion for the call site.

Ktlint enables trailing commas on call site by default for the same reasons as on the declaration site, however, while IntelliJ can be configured to allow or disallow trailing commas, Ktlint can only be configured to enforce or disallow them. Consistency is preferred, which is honestly a good decision I think.

Now, enforcing trailing commas does make notation like

myFunction(
  singleArg,
)

appear. Which is debatably ugly and can hurt our ability to read it well, however it can often be rewritten like:

myFunction(singleArg)

or

myFunction(
  arg = singleArg,
)

Having trailing commas for function calls with many (optional) named arguments is still a must for me:

createValueColumn(
    name = name,
    values = values,
    type = getValuesType(
        values = values,
        type = typeOf<T>(),
        infer = infer,
    ),
)

Being able to reorder arguments like these or add new ones is something that occurs a lot and it can save headaches with merge conflicts if every line simply ends with a comma.

So, to conclude, I think we should follow kotlin-juypter and just enable them both on the call- and the declaration site.