Fix ktlint-disable directive on package statement #1038
Conversation
* Split unit tests for error suppression into multiple tests * Extract the creation of the SuppressionHints to extension methods for readability and reuseability
…dentTest.kt.txt Avoid breaking the build as long as the ktlint-disable directive on the package statement is not recognized. See pull request pinterest#1038
ktlint-core/src/test/kotlin/com/pinterest/ktlint/core/ErrorSuppressionTest.kt
Show resolved
Hide resolved
| PsiWhiteSpace? | ||
| )?.let { it.node.startOffset + it.text.lastIndexOf('\n') + 1 } ?: 0 | ||
| result.add(SuppressionHint(IntRange(lineStart, node.startOffset), HashSet(args))) | ||
| if (node.isDisabledOnPackageStatement()) { |
There was a problem hiding this comment.
KtLint readme on this case is not very clear.
import package.* // ktlint-disable
Should disable any rule check on this particular line.
It doesn't make sense to disable all the rules for the whole file - better to either not pass it to KtLint or exclude it via GLOB pattern. To disable all the rules for the region, better to use:
/* ktlint-disable */
<some code>
/* ktlint-enable */So, for the case <some code> // ktlint-disable please use createLineDisableSupressionHint and pass emptySet() as disabledRules.
There was a problem hiding this comment.
I really like puzzles. But this review remark really confused me at first. I have the feeling that we were discussing different, albeit related, topics. Let's try to unravel this.
KtLint readme on this case is not very clear.
import package.* // ktlint-disableShould disable any rule check on this particular line.
This code example is confusing for different reasons:
- There is no apparent reason why this statement should be excluded by ktlint as there is no rule violated.
- The word
packageafterimporttook precedence for me. So I read this statement as being the package statement:
package com.example.mypackage // ktlint-disable
I have modified this.
It doesn't make sense to disable all the rules for the whole file - better to either not pass it to KtLint or exclude it via GLOB pattern.
It is only possible to follow this approach when you know which files need to be excluded from processing by ktlint. In bigger code bases with which you are not familiar you may just not know which files would need to be excluded. When you are able to specify it explicitly in the file itself, you can also document the reasoning for this. This approach allows to lint/format the entire code base without having to know the files which need to be excluded.
So, for the case
<some code> // ktlint-disableplease usecreateLineDisableSupressionHintand passemptySet()asdisabledRules.
I have removed the code for the ktlint-disable directive on the package statement as I no longer needed it after fixing some other bugs.
I found out that certain bugs are caused by the fact that some rules perform a kind of initialization as soon as the rootNode is visited. If for some reasons, for example disabling a rule, the rootNode is not visited then the rule would not have been initialized correctly. I have split initialization and visiting.
Next problem was that the IndentationRule modifies the ASTNode hierarchy by inserting additional node. Disabling the indentation rule was not always recognized by the SupressedRegionLocator. I have fixed and simplified this locator.
The changes can best be reviewed per commit.
Make a clearer distinction between the initialization of a rule versus processing (e.g. visiting) the rule with a node. A rule that implements the Rule.Modifier.InitializeRoot interface will be called with the rootNode to initialize the rule. After that the rule will be visited for each node (including the root node) unless the node is suppressed by a ktlint-disable directive.
When starting a new block in which a rule is disabled, then define the range of this block from the startOffset of that block until eternity (Int.MAX_VALUE). When the block is closed explicitly by a ktlint-enable directive, or when closing all open blocks at the end of the collect phase, the end position of the block is overwritten with the offset of the end of the file. This fixes a very specific bug in the IndentationRule which on occasion modifies the structure of the ASTNodes by inserting new nodes. As of such, the rootNode can contain offsets which are not equal to zero but which still need to be suppressed. This bug can not be tested in the unit test of the SupressedRegionLocator but is verified in the unit test IndentationRuleTest.
The special disable directive on the package statement is no longer required because disabling via the block comment has been improved. The latter is more clear and consistent.
There was a problem hiding this comment.
Thank you for your effort, but it seems too much changes trying to fix #1029
This issue could be fixed by changing SuppressedRegionLocator.kt#74 into:
if (node is PsiComment && node.prevSibling != null) {that will ignore all lines containing only // ktlint-disabled comment.
|
As stated in my previous comment, my changes also fix other problems. Please take that into account as well. |
|
PR is too far out of sync with current master. |
* Fix multiline string indentation The indenting margin is determined analog to the trimIndent function. Spaces and tabs are both handled as single indent characters. The indentation of the closing quotes is not relevant unless the opening and closing quotes are on different lines but does not contain any content which is weird but still valid Kotlin code. The 'IndentationRuleTrimIndentTest' is added to clarify and validate the behavior of the trimIndent function. Content of a multiline string should not be indented with respect to the closing quotes (note that the opening quotes can be on a previous line, for example after an assignment). File 'format-raw-string-trim-indent.kt.spec' is changed for following reasons: * Example code was not valid Kotlin code * Changed a comment in an example to explain why the autocorrected code actually looks worse than the original. This will be fixed in a next PR. * Fix code which does not comply to changed indentation rules * Rename file IndentationRuleTrimIndentTest.kt to IndentationRuleTrimIndentTest.kt.txt Avoid breaking the build as long as the ktlint-disable directive on the package statement is not recognized. See pull request #1038 * Fix indentation in unit tests * Remove changing the indentation margin of a multiline raw string literal except for the line with the closing quotes * Change message when closing quotes of multiline raw string literal are incorrectly indented * Remove IndentationRuleTrimIndentTest The indentation margin is no longer been altered by the indent rule. So there is no need anymore to verify the behavior of the trimIndent function. Co-authored-by: Paul Dingemans <pdingemans@bol.com>
methods for readability and reuseability
Description
According to the documentation the
ktlint-disablecan be appended as an EOL comment to indicate that all rules for this file should be disabled. However that didn't workAdding it as block comment before the package statement resulted in an internal error on the import rule. I have not tried to fix that.
Now it is possible to:
// ktlint-disableafter the package statement// ktlint-disable rule-idafter the package statement