-
-
Notifications
You must be signed in to change notification settings - Fork 793
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
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
@@ -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") |
There was a problem hiding this comment.
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).
There was a problem hiding this 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()) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this 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 👍
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.