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

Upgrade ktlint to 0.47.1 #5312

Merged
merged 12 commits into from Oct 7, 2022
Merged

Upgrade ktlint to 0.47.1 #5312

merged 12 commits into from Oct 7, 2022

Conversation

chao2zhang
Copy link
Member

@chao2zhang chao2zhang commented Sep 18, 2022

Fixes #5281

Release note:

Majority of the changes are:

Comment on lines +33 to +37
if (contains(File.separatorChar)) {
substringAfterLast(File.separatorChar)
} else {
this
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This violation is detected once pinterest/ktlint#828 is addressed.

Comment on lines +57 to +80
wrapping.beforeFirstNode(computeEditorConfigProperties())
root.node.visitASTNodes()
wrapping.afterLastNode()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See Rule lifecycle hooks / deprecate RunOnRootOnly visitor modifier in https://github.com/pinterest/ktlint/releases/tag/0.47.0

Comment on lines -109 to -119
open fun getTextLocationForViolation(node: ASTNode, offset: Int) =
TextLocation(node.startOffset, node.psi.endOffset)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is removed because no rules in Ktlint is RunOnRootNodeOnly. RunOnRootNodeOnly itself is also deprecated and to be removed in 0.48.0


private fun ruleShouldOnlyRunOnFileNode(node: ASTNode) =
runOnRootNodeOnly && node !is FileASTNode
private fun ASTNode.isNotDummyElement(): Boolean {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function is renamed to improve readability and consistency

Comment on lines +123 to +121
TrailingCommaOnCallSite(config), // in standard ruleset but not enabled by default
TrailingCommaOnDeclarationSite(config), // in standard ruleset but not enabled by default
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TrailingCommaRule is split into two separate rules - pinterest/ktlint#1555

I would propose to keep them disabled because it may sparkle endless conversation within a medium-large sized team

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't they be enforced by default on declaration site in line with the Kotlin coding conventions?

https://kotlinlang.org/docs/coding-conventions.html#trailing-commas

The Kotlin style guide encourages the use of trailing commas at the declaration site

I also think they should be enabled by default for the call site for the reasons outlined in the coding conventions:

  • 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's easy to disable for teams that don't want this. I don't think there's a strong reason to deviate from ktlint's default behaviour - and anyone using ktlint directly would have to have the same conversation if they don't like them. It's also easy to disable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there's a strong reason to deviate from ktlint's default behaviour

Agree on being consistent with KtLint's behavior

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me keep this disabled for now in a separate PR because it will cause at least 200+ violations across the codebase - I will create a separate PR to demonstrate what it looks like.

Copy link
Member Author

@chao2zhang chao2zhang Oct 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#5386 is the PR to be consistent with ktlint

Shouldn't they be enforced by default on declaration site in line with the Kotlin coding conventions?
Yes, and ktlint's default configuration still diverges from kotlin coding convention. We could configure these rules in a way that conforms to the kotlin coding convention, namely:

  • TrailingCommaOnDeclarationSite: active = true, useTrailingCommaOnDeclarationSite = true
  • TrailingCommaOnDeclarationSite: active = false

Comment on lines +147 to +145
getSortedRules().forEach { rule ->
rule.visit(root)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am trying to move more logic into FormattingRule itself to prepare for #5192 (comment)

@codecov
Copy link

codecov bot commented Sep 30, 2022

Codecov Report

Merging #5312 (68d4159) into main (7027c27) will decrease coverage by 0.01%.
The diff coverage is 87.50%.

@@             Coverage Diff              @@
##               main    #5312      +/-   ##
============================================
- Coverage     86.11%   86.09%   -0.02%     
+ Complexity     3643     3637       -6     
============================================
  Files           515      516       +1     
  Lines         12085    12098      +13     
  Branches       2161     2156       -5     
============================================
+ Hits          10407    10416       +9     
- Misses          609      613       +4     
  Partials       1069     1069              
Impacted Files Coverage Δ
...turbosch/detekt/formatting/wrappers/Indentation.kt 100.00% <ø> (ø)
.../arturbosch/detekt/formatting/wrappers/Wrapping.kt 100.00% <ø> (ø)
...etekt/rules/coroutines/RedundantSuspendModifier.kt 63.63% <40.00%> (-0.65%) ⬇️
...les/style/ExplicitCollectionElementAccessMethod.kt 75.55% <50.00%> (-1.72%) ⬇️
...b/arturbosch/detekt/rules/complexity/LongMethod.kt 92.15% <66.66%> (+0.15%) ⬆️
...urbosch/detekt/rules/style/NullableBooleanCheck.kt 81.81% <66.66%> (+0.86%) ⬆️
...lab/arturbosch/detekt/formatting/FormattingRule.kt 92.30% <91.48%> (-5.70%) ⬇️
...itlab/arturbosch/detekt/api/internal/Signatures.kt 88.88% <100.00%> (+0.51%) ⬆️
...ab/arturbosch/detekt/formatting/KtLintMultiRule.kt 98.73% <100.00%> (+1.99%) ⬆️
...ekt/formatting/wrappers/TrailingCommaOnCallSite.kt 100.00% <100.00%> (ø)
... and 9 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@@ -79,15 +79,19 @@ class AnnotationExcluderSpec(private val env: KotlinCoreEnvironment) {
@Test
fun `should exclude when the annotation was found with SplitPattern`() {
val (file, ktAnnotation) = createKtFile("@SinceKotlin")
val excluder = @Suppress("DEPRECATION") AnnotationExcluder(file, SplitPattern("SinceKotlin"))

@Suppress("DEPRECATION")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These seems like a false positive from ktlint, isn't it? They are forcing you to increase the annotation scope.

@@ -179,11 +179,14 @@ formatting:
StringTemplate:
active: true
autoCorrect: true
TrailingComma:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is kind of a breaking change. We should note it. Or adding it to the deprecation list. We don't have a way to deprecated plugin properties... we could use the "standard" one as a workaround for now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do - I made this breaking change in ktlint 🙈 Let me add it to the deprecation list

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added to deprecation.properties

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it won't work. deprecation.properties only takes care of deprecated rule properties, not the legacy properties that don't work.

Copy link
Member

@cortinico cortinico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change looks good to me, modulo the open comments

Copy link
Member

@3flex 3flex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, just need to match the default config with ktlint.

@chao2zhang
Copy link
Member Author

chao2zhang commented Oct 10, 2022

With regards to #5312 (comment), it looks like ktlint added a test specifically to ensure that annotation with parameters on expressions are put at a separate line pinterest/ktlint@069583e

I don't hold a strong opinion on this, as it does not conflict with Kotlin official coding convention.

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

Successfully merging this pull request may close these issues.

Update ktlint to 0.47.x
4 participants