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

Indent rule cannot be suppressed via ktlint-disable directives in the middle of a file. #631

Closed
shashachu opened this issue Oct 29, 2019 · 6 comments · Fixed by #1547
Closed

Comments

@shashachu
Copy link
Contributor

shashachu commented Oct 29, 2019

Since it operates on the root node, it can only be disabled via a // ktlint-disable directive at the very top of the file.

The final newline rule exhibits the same behavior but that's less confusing.

@shashachu shashachu changed the title Experimental indent rule cannot be suppressed via ktlint-disable directives Experimental indent rule cannot be suppressed via ktlint-disable directives in the middle of a file. Oct 29, 2019
@shashachu
Copy link
Contributor Author

Maybe this is desired behavior since it can only run on the entire file in order to properly calculate indent.

@Tapchicoma
Copy link
Collaborator

Imho, this rule should be possible to disable for some parts of the file. Though not sure how often it will be used, so I would propose to wait for other users requests to implement this.

@yonigibbs
Copy link

yonigibbs commented May 26, 2020

I agree, I think it would be useful to be able to turn this feature off for a specific line. Below is something replicating our scenario. Whether the example below should be additionally logged as a separate issue I'm not sure: let me know if you'd like me to log it.

fun foo() {
    val ABCDEFGHIJKLMNOPQRSTUVWXYZ1ABCDEFGHIJKLMNOPQRSTUVWXYZ1 = "a"
    val ABCDEFGHIJKLMNOPQRSTUVWXYZ2ABCDEFGHIJKLMNOPQRSTUVWXYZ2 = "b"
    val ABCDEFGHIJKLMNOPQRSTUVWXYZ3ABCDEFGHIJKLMNOPQRSTUVWXYZ3 = "c"

    if (bar(
            ABCDEFGHIJKLMNOPQRSTUVWXYZ1ABCDEFGHIJKLMNOPQRSTUVWXYZ1,
            ABCDEFGHIJKLMNOPQRSTUVWXYZ2ABCDEFGHIJKLMNOPQRSTUVWXYZ2,
            ABCDEFGHIJKLMNOPQRSTUVWXYZ3ABCDEFGHIJKLMNOPQRSTUVWXYZ3
        )
    ) {
        println("test")
    }
}

fun bar(x: String, y: String, z: String) = true

The code above is how IntelliJ formats it for us. The error from ktlint is: Unexpected indentation (12) (should be 8) (experimental:indent)

When we ask ktlint to fix the code, it changes it to this:

    if (bar(
        ABCDEFGHIJKLMNOPQRSTUVWXYZ1ABCDEFGHIJKLMNOPQRSTUVWXYZ1,
        ABCDEFGHIJKLMNOPQRSTUVWXYZ2ABCDEFGHIJKLMNOPQRSTUVWXYZ2,
        ABCDEFGHIJKLMNOPQRSTUVWXYZ3ABCDEFGHIJKLMNOPQRSTUVWXYZ3
    )
    ) {
        println("test")
    }

I don't think the fix above looks right (and it disagrees with what IntelliJ does), which is why we wanted to disable the rule just for that line.

@paul-dingemans
Copy link
Collaborator

Is already fixed by #796

@Tapchicoma
Copy link
Collaborator

@paul-dingemans how has #796 fixed it? indent rule still could not be disabled for some parts of the file.

@paul-dingemans
Copy link
Collaborator

I meant that the example below has already been fixed by another issue:

fun foo() {
    val ABCDEFGHIJKLMNOPQRSTUVWXYZ1ABCDEFGHIJKLMNOPQRSTUVWXYZ1 = "a"
    val ABCDEFGHIJKLMNOPQRSTUVWXYZ2ABCDEFGHIJKLMNOPQRSTUVWXYZ2 = "b"
    val ABCDEFGHIJKLMNOPQRSTUVWXYZ3ABCDEFGHIJKLMNOPQRSTUVWXYZ3 = "c"

    if (bar(
            ABCDEFGHIJKLMNOPQRSTUVWXYZ1ABCDEFGHIJKLMNOPQRSTUVWXYZ1,
            ABCDEFGHIJKLMNOPQRSTUVWXYZ2ABCDEFGHIJKLMNOPQRSTUVWXYZ2,
            ABCDEFGHIJKLMNOPQRSTUVWXYZ3ABCDEFGHIJKLMNOPQRSTUVWXYZ3
        )
    ) {
        println("test")
    }
}

fun bar(x: String, y: String, z: String) = true

Based on the existing unit test below, I just copied the issue number from the comment without checking it thoroughly:

    // https://github.com/pinterest/ktlint/issues/796
    @Test
    fun `lint if-condition with multiline call expression is indented properly`() {
        val code =
            """
            private val gpsRegion =
                if (permissionHandler.isPermissionGranted(
                        context, Manifest.permission.ACCESS_FINE_LOCATION
                    )
                ) {
                    // stuff
                }
            """.trimIndent()
        assertThat(IndentationRule().lint(code)).isEmpty()
    }

The commit message in which this change was made refers to #833 which in turn refers again to #796.

@romtsn romtsn changed the title Experimental indent rule cannot be suppressed via ktlint-disable directives in the middle of a file. Indent rule cannot be suppressed via ktlint-disable directives in the middle of a file. May 28, 2021
@paul-dingemans paul-dingemans added this to the 0.47.0 milestone Jul 14, 2022
paul-dingemans added a commit to paul-dingemans/ktlint that referenced this issue Jul 17, 2022
paul-dingemans added a commit to paul-dingemans/ktlint that referenced this issue Jul 17, 2022
paul-dingemans added a commit that referenced this issue Jul 17, 2022
Up until ktlint 0.46 the Rule class provided only one life cycle hook. This "visit" hook was called in a depth-first-approach on all nodes in the file. A rule like the IndentationRule used the RunOnRootOnly visitor modifier to call this lifecycle hook for the root node only in combination with an alternative way of traversing the ASTNodes. Downside of this approach was that suppression of the rule on blocks inside a file was not possible (#631). More generically, this applied to all rules, applying alternative traversals of the AST.

The Rule class now offers new life cycle hooks:

beforeFirstNode: This method is called once before the first node is visited. It can be used to initialize the state of the rule before processing of nodes starts. The ".editorconfig" properties (including overrides) are provided as parameter.
beforeVisitChildNodes: This method is called on a node in AST before visiting its child nodes. This is repeated recursively for the child nodes resulting in a depth first traversal of the AST. This method is the equivalent of the "visit" life cycle hooks. However, note that in KtLint 0.48, the UserData of the rootnode no longer provides access to the ".editorconfig" properties. This method can be used to emit Lint Violations and to autocorrect if applicable.
afterVisitChildNodes: This method is called on a node in AST after all its child nodes have been visited. This method can be used to emit Lint Violations and to autocorrect if applicable.
afterLastNode: This method is called once after the last node in the AST is visited. It can be used for teardown of the state of the rule.
The "visit" life cycle hook will be removed in Ktlint 0.48. In KtLint 0.47 the "visit" life cycle hook will be called only when hook "beforeVisitChildNodes" is not overridden. It is recommended to migrate to the new lifecycle hooks in KtLint 0.47. Please create an issue, in case you need additional assistence to implement the new life cycle hooks in your rules.

Closes #631

Highlight of changes included:
* Extend Rule hooks with beforeFirstNode, beforeVisitChildNodes, afterVisitChildNodes and afterLastNode and deprecating visit.
* Simplification of logic
  - Remove node parameter from VisitorProvider result to simplify code
  - Simplify condition to rebuild supressed region locator
  - Checking for a suppressed region on the root node is not necessary. By default, the root node (e.g. the node with ElementType FILE) can not be suppressed. A '@file:Suppress' annotation on top of the file is already contained in a non-root node.
  - Remove indentation logic of complex arguments in ArgumentListWrappingRule and ParameterListWrappingRule as this is the responsibility of the IndentationRule
* Remove concurrent visitor provider: Despite the name, the concurrent visitor is not faster than the sequential visitor as each file is processed on a single thread. There seem to be no other benefits for having two different visitor providers. Inlined the sequentialVisitor.
* Fix early return when no rules have to be run
* Extract PreparedCode and move PsiFileFactory to this vlass
* Move logic to check whether to run on root node only or on all nodes from VisitorProvider to Ktlint. The visitor provider is only responsible for running the relevant rules in the correct order. On which node is to be executed is not relevant.
* Deprecate the RunOnRootNodeOnly visitor modifier: This visitor modifier was typically used to run a custom node visit algorithm using the root node and the generic "package.visit" methods. The "package.visit" method however did not support rule suppression. With the new lifecycle hooks on the Rule class the visit modifier has become
obsolete.
* Refactor rules to comply with new life cycle hooks. Some rules needed a bigger refactoring to achieve this.
* Provide EditorConfigProperties directly instead of via ASTNode UserData: Prepare to remove ".editorconfig" properties from the UserData in ktlint 0.48. Those properties are only required on the root node but the "getUserData" method can be called on any node. The new Rule lifecycle hooks "beforeFirstNode" provides the ".editorconfig" properties directly to the rule. This contract is cleaner and more explicit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants