Skip to content
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

Bump KtLint to 0.43.2 and add TrailingComma rule #4227

Merged
merged 5 commits into from
Jan 3, 2022

Conversation

cortinico
Copy link
Member

@cortinico cortinico commented Nov 3, 2021

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:

Fixes #2398

@cortinico cortinico added rules dependencies Pull requests that update a dependency file labels Nov 3, 2021
@cortinico cortinico added this to the 1.19.0 milestone Nov 3, 2021
@codecov
Copy link

codecov bot commented Nov 3, 2021

Codecov Report

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

Impacted file tree graph

@@             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
Copy link
Member

3flex commented Nov 3, 2021

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
Copy link
Member Author

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
Copy link
Member

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

@ZacSweers
Copy link
Contributor

Another possible issue - fyi pinterest/ktlint#1274

@cortinico
Copy link
Member Author

Another possible issue - fyi pinterest/ktlint#1274

Added to the list 👍

@BraisGabin BraisGabin modified the milestones: 1.19.0, 1.20.0 Nov 25, 2021
@cortinico cortinico changed the title Bump KtLint to 0.43.0 and add TrailingComma rule Bump KtLint to 0.43.1 and add TrailingComma rule Dec 1, 2021
@romtsn
Copy link

romtsn commented Dec 1, 2021

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.

@cortinico cortinico changed the title Bump KtLint to 0.43.1 and add TrailingComma rule Bump KtLint to 0.43.2 and add TrailingComma rule Dec 2, 2021
@cortinico cortinico added the notable changes Marker for notable changes in the changelog label Dec 2, 2021
@3flex
Copy link
Member

3flex commented Dec 18, 2021

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?

…formatting/wrappers/TrailingComma.kt

Co-authored-by: Matthew Haughton <3flex@users.noreply.github.com>
@cortinico
Copy link
Member Author

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 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file notable changes Marker for notable changes in the changelog rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New rule: (Require|Forbid)TrailingCommas
6 participants