diffplug / spotless

Keep your code spotless
Apache License 2.0
4.46k stars 449 forks source link

take toggleOffOn into account for linters #719

Open MarijanGazica opened 3 years ago

MarijanGazica commented 3 years ago

First of all, I'm expecting a solution to this to be something extremely obvious but here it is. Spotless setup in the project looks like this:

apply plugin: "com.diffplug.spotless"

spotless {

    ext.targetBranch = "develop"
    if (project.hasProperty('target_branch') && project.getProperty('target_branch')?.trim()) {
        targetBranch = project.getProperty('target_branch')
    }

    ratchetFrom "origin/$targetBranch"

    kotlin {
        target '**/*.kt'
        trimTrailingWhitespace()
        indentWithSpaces()
        toggleOffOn('format:off', 'format:on')

        ktlint().userData([
                'max_line_length': '120',
                'disabled_rules' : 'import-ordering' //https://github.com/pinterest/ktlint/issues/527, https://youtrack.jetbrains.com/issue/KT-10974
        ])
    }
}

The problem occurs when trying to exclude a part of the code using spotless:off and spotless:on I also tried (as seen in the snippet above) with different tags but running ./gradlew spotlessCheck still fails.

The part where it fails looks something like this:


    // format:off
    private fun functionName(): ReturnType = when (this) {
        // A bunch of stuff, one line is >120 chars, our line limit
    }
    // format:on

Reason for failure:

java.lang.AssertionError: Error on line: 70, column: 1
Exceeded max line length (120)

What is it that I'm missing here?

nedtwigg commented 3 years ago

If ktlint responded to exceeding max line length by autowrapping, then the off/on tags would work. But ktlint apparently doesn't respond by autowrapping, it just yells.

For everything that ktlint "just fixes", the off/on tags will work. For things that ktlint doesn't "just fix", and instead yells about, these tags will not work.

The fix would be to make ktlint respond to "exceed max line lingth" by autowrapping, rather than throwing an exception.

Details

Unfortunately, this is behaving correctly. You can split code quality tools into two categories:

The great thing about "problem solvers" is that you can compose them. Your configuration block has several (trimTrailingWhitespace, indentWithSpaces, ktlint). The way that toggleOffOn works is by storing what is between the off/on tags before the steps run, and then putting it back at the end. Spotless is all about composing problem solvers.

The terrible thing about "problem yellers" is that you can't compose them. They don't fix problems, they just yell about them. In the case of ktlint, it just yells about the first thing that it finds. If you fix the max line length on line 70, it will next yell about the problem on line 77. That's a big limitation of "yellers" as opposed to "fixers".

Most tools are either problem yellers or solvers. Ktlint is the rare exception, where it seems to be 50/50. Which means that half the time it works great with Spotless, but the other half of the time it just shouts "everybody stop" and people are disappointed that it doesn't play nice with the rest of the Spotless infrastructure.

One option is to look at ktfmt, a recent tool from facebook which is all "solver", and no "yeller". 1

[1] All "solvers" are at least a little "yeller". For example, if you pass C++ to ktfmt, it will throw a parsing exception and yell. But most autoformatting tools use yelling as a last resort. ktlint is the rare case that uses yelling for many fixable issues like "line is too long".

MarijanGazica commented 3 years ago

Sorry for the late reply, just wanted to thank you for a great response. I'll look into the direction you pointed

nedtwigg commented 2 years ago

We can make this work in the future. Probably the spotless:off / spotless:on mechanism isn't the right one, but maybe instead spotless:lintoff:CODE_FOR_LINT. For each lint, search for that string on the affected lines, and if it's there suppress it.