-
-
Notifications
You must be signed in to change notification settings - Fork 756
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
Warnings as errors #3646
Warnings as errors #3646
Conversation
synchronized(Extensions.getRootArea()) { | ||
arrayOf(project.extensionArea, Extensions.getRootArea()) | ||
synchronized(@Suppress("DEPRECATION") Extensions.getRootArea()) { | ||
arrayOf(project.extensionArea, @Suppress("DEPRECATION") Extensions.getRootArea()) |
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.
I have no idea what this code does and I can't find any alternative to this. The other suppressions are safe because we are suppressing our own deprecations and that's fine. But this case is different. What do you think? The problem is that if we don't fix this we can't enable warningsAsErrors
again.
Other option would be to disable warningsAsErrors
just for this module.
What do you think?
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.
The code here seems to enable AST mutations.
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.
😕 so ktlint still have that deprecated code... Any idea about how to fix this deprecation properly? How should we proceed 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.
The replacement can be found here https://github.com/JetBrains/intellij-community/blob/master/platform/extensions/src/com/intellij/openapi/extensions/Extensions.java#L34. Would you mind trying it and verify the AST mutation still works?
Codecov Report
@@ Coverage Diff @@
## main #3646 +/- ##
============================================
- Coverage 78.12% 78.11% -0.02%
+ Complexity 2837 2835 -2
============================================
Files 467 467
Lines 9143 9143
Branches 1737 1737
============================================
- Hits 7143 7142 -1
- Misses 1058 1059 +1
Partials 942 942
Continue to review full report at Codecov.
|
2158c41
to
0b78c89
Compare
This was removed in #2981 for an issue with kotlin 1.4 but that issue is fixed now so it's safe to enable it again
0b78c89
to
b732719
Compare
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.
We can delay Extensions.getRootArea()
to a follow-PR. The urgency is to avoid adding new violations as warnings.
I created #3683 so we don't forget about it. |
* Deprecate Location.file in favor of Location.filePath * Fix warnings * Enable warningsAsErrors again This was removed in detekt#2981 for an issue with kotlin 1.4 but that issue is fixed now so it's safe to enable it again
This PR removes all the current warnigns in the project and enables
warningsAsErrors
in CI as before #2981. It was removed for some problems with Kotlin 1.4 that are fixed now so it's safe to add it again.