Bump KtLint to 0.43.2 and add TrailingComma rule

[cortinico]

See https://github.com/pinterest/ktlint/releases/tag/0.43.0 for the full release note. I've also wrapped the new experimental TrailingComma rule.

I have to mention that I'm not entirely happy with the naming of the config keys:

    allowTrailingComma: false
    allowTrailingCommaOnCallSite: false

As the rule is doing more than just allowing. Setting the value to true is effectively enforcing trailing commas, while setting it to false (the default) is forbidding it. I've anyway followed the same name that ktlint is having.

Plus there seems to be some regressions on KtLint 0.43.0:

• Trailing assignments on next line are missing continuation indent in 0.43.0 pinterest/ktlint#1268
• Split type arguments break 0.43.0 pinterest/ktlint#1267
• KtLint 0.43.0 is missing requisite --add-opens to work in JDK 16+ pinterest/ktlint#1274

Fixes #2398

[codecov]

Codecov Report

Merging #4227 (3b23422) into main (d111fe4) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #4227      +/-   ##
============================================
+ Coverage     84.28%   84.34%   +0.06%     
- Complexity     3282     3299      +17     
============================================
  Files           474      473       -1     
  Lines         10429    10533     +104     
  Branches       1861     1885      +24     
============================================
+ Hits           8790     8884      +94     
- Misses          670      671       +1     
- Partials        969      978       +9     

Impacted Files	Coverage Δ
...ab/arturbosch/detekt/formatting/KtLintMultiRule.kt	95.89% <100.00%> (+0.05%)	⬆️
...rbosch/detekt/formatting/wrappers/TrailingComma.kt	100.00% <100.00%> (ø)
...b/arturbosch/detekt/core/tooling/AnalysisFacade.kt	62.85% <0.00%> (-3.81%)	⬇️
...h/detekt/rules/style/MultilineLambdaItParameter.kt	88.88% <0.00%> (-3.71%)	⬇️
...sch/detekt/rules/style/UnnecessaryAbstractClass.kt	86.66% <0.00%> (-0.29%)	⬇️
...gitlab/arturbosch/detekt/core/tooling/Lifecycle.kt	100.00% <0.00%> (ø)
...ab/arturbosch/detekt/core/config/Configurations.kt	84.37% <0.00%> (ø)
...detekt/tooling/api/DefaultConfigurationProvider.kt	100.00% <0.00%> (ø)
...bosch/detekt/core/tooling/DefaultConfigProvider.kt	80.00% <0.00%> (ø)
...urbosch/detekt/rules/style/ExpressionBodySyntax.kt	83.33% <0.00%> (ø)
... and 21 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d111fe4...3b23422. Read the comment docs.

[3flex]

I have to mention that I'm not entirely happy with the naming of the config keys

Funnily enough that exact feedback was also provided on the ktlint issue, and they said they followed the terminology used in IntelliJ (which also says "allow" but acts as "require").

So all tooling is going to have a confusing name for this but at least it's consistent :/

IMHO it's better to change the name of the config option to reflect the effect it has.

[cortinico]

IMHO it's better to change the name of the config option to reflect the effect it has.

I would agree on this, if this would be a Detekt first party rule. But as we're wrapping a 3rd party rule, let's stick to the naming already decided by others.

[BraisGabin]

I vote to don't merge until those regressions are fixed.

[ZacSweers]

Another possible issue - fyi pinterest/ktlint#1274

[cortinico]

Another possible issue - fyi pinterest/ktlint#1274

Added to the list 👍

[romtsn]

I have to mention that I'm not entirely happy with the naming of the config keys

Funnily enough that exact feedback was also provided on the ktlint issue, and they said they followed the terminology used in IntelliJ (which also says "allow" but acts as "require").

So all tooling is going to have a confusing name for this but at least it's consistent :/

IMHO it's better to change the name of the config option to reflect the effect it has.

the reasoning behind that is we want a centralized configuration for both ktlint and IJ (so they don't conflict and confuse people) which appears to be .editorconfig, and there it's named like that. I'd happily rename it to something more meaningful if it was under our control. Maybe worth throwing an issue on youtrack.

[3flex]

I vote to don't merge until those regressions are fixed.

It's been 6 weeks and not much movement on those regressions. Generally we are prompt at updating dependencies and we don't directly control what happens in ktlint, perhaps it's best to update but make sure docs make clear how ktlint version can be overridden when using the wrapper?

[cortinico]

It's been 6 weeks and not much movement on those regressions. Generally we are prompt at updating dependencies and we don't directly control what happens in ktlint, perhaps it's best to update but make sure docs make clear how ktlint version can be overridden when using the wrapper?

Agree. I think it's safe to merge this. Should a point release on KtLint's end happen, we're going to include it inside 1.20.0 regardless 👍