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 version 0.45.1 and implement new rules #4645

Merged
merged 6 commits into from Mar 29, 2022

Conversation

kvn-stgl
Copy link
Contributor

See https://github.com/pinterest/ktlint/releases/tag/0.45.0 for full release notes. I've also wrapped the new (experimental) rules in this version.

The PR is a follow-up PR of #4641

I will open this PR as a draft, because currently the build doesn't work (see pinterest/ktlint#1421). Feel free to use this PR for further work on it.

@codecov
Copy link

codecov bot commented Mar 21, 2022

Codecov Report

Merging #4645 (f79782b) into main (14c0f11) will increase coverage by 84.57%.
The diff coverage is 97.33%.

@@             Coverage Diff             @@
##             main    #4645       +/-   ##
===========================================
+ Coverage        0   84.57%   +84.57%     
- Complexity      0     3397     +3397     
===========================================
  Files           0      490      +490     
  Lines           0    11250    +11250     
  Branches        0     2040     +2040     
===========================================
+ Hits            0     9515     +9515     
- Misses          0      701      +701     
- Partials        0     1034     +1034     
Impacted Files Coverage Δ
...n/io/github/detekt/report/sarif/RuleDescriptors.kt 33.33% <60.00%> (ø)
...lab/arturbosch/detekt/formatting/FormattingRule.kt 97.95% <100.00%> (ø)
...ab/arturbosch/detekt/formatting/KtLintMultiRule.kt 96.38% <100.00%> (ø)
...detekt/formatting/wrappers/ArgumentListWrapping.kt 100.00% <100.00%> (ø)
...tting/wrappers/BlockCommentInitialStarAlignment.kt 100.00% <100.00%> (ø)
...osch/detekt/formatting/wrappers/CommentWrapping.kt 100.00% <100.00%> (ø)
.../formatting/wrappers/DiscouragedCommentLocation.kt 100.00% <100.00%> (ø)
...urbosch/detekt/formatting/wrappers/FinalNewline.kt 100.00% <100.00%> (ø)
...ch/detekt/formatting/wrappers/FunKeywordSpacing.kt 100.00% <100.00%> (ø)
...ormatting/wrappers/FunctionTypeReferenceSpacing.kt 100.00% <100.00%> (ø)
... and 489 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 14c0f11...f79782b. Read the comment docs.

@kvn-stgl kvn-stgl marked this pull request as ready for review March 21, 2022 22:35
@@ -89,8 +83,14 @@ abstract class FormattingRule(config: Config) : Rule(config) {

// KtLint 0.44.0 is assuming that KtLint.EDITOR_CONFIG_USER_DATA_KEY is available on all the nodes.
// If not, it crashes with a NPE. Here we're patching their behavior.
// This block is deprecated and will be removed in KtLint 0.46. But we have to suppress the
// deprecation warning because the ci runs with -Werror.
@Suppress("DEPRECATION")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest, I'm not really happy with this solution. Also I wanted to declare the ´overrideEditorConfig()` method as deprecated. But sadly each of deprecation will lead to a warning which is handled as error in the ci pipeline so I was forced to suppress this warning (without this code snippet the build will not succeed).

@kvn-stgl kvn-stgl changed the title Bump ktlint to version 0.45.0 and implement new rules Bump ktlint to version 0.45.1 and implement new rules Mar 21, 2022
@kvn-stgl kvn-stgl mentioned this pull request Mar 22, 2022
1 task
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.

Amazing work! 🎉 Left a couple of comments but we're good to go with this

node.putUserData(KtLint.EDITOR_CONFIG_USER_DATA_KEY, fromMap(emptyMap()))
node.putUserData(
KtLint.EDITOR_CONFIG_USER_DATA_KEY,
com.pinterest.ktlint.core.EditorConfig.Companion.fromMap(emptyMap())
Copy link
Member

Choose a reason for hiding this comment

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

Please keep the import as it was before

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But then I have to suppress the warning over the entire file.
Without, the build will throw a warning like w: .../FormattingRule.kt: (3, 34): 'EditorConfig' is deprecated. Marked for removal ... and the code will probably not be able to build on the ci.
It's not aesthetic but I think @file:Suppress("DEPRECATION") is the more dirty way here.

Copy link
Member

Choose a reason for hiding this comment

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

Yup makes sense. Thanks for clarifying 👍

/**
* See <a href="https://ktlint.github.io/#rule-indentation">ktlint-website</a> for documentation.
*/
@ActiveByDefault(since = "1.20.0")
Copy link
Member

Choose a reason for hiding this comment

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

Why is this active while the others are not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because of the changelog:

All wrapping logic is moved from the indent rule to the new rule wrapping (as part of the standard ruleset). In case you currently have disabled the indent rule, you may want to reconsider whether this is still necessary or that you also want to disable the new wrapping rule to keep the status quo. Both rules can be run independent of each other.

So the WrappingRule is in the standard ruleset and active by default (as far as I understand). Each other rule is in the experimental ruleset and should not be active by default.

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.

This is good for me to go 👍

@cortinico cortinico merged commit 4833c49 into detekt:main Mar 29, 2022
@kvn-stgl kvn-stgl deleted the bump-ktlint-0450 branch April 5, 2022 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
formatting notable changes Marker for notable changes in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants