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

Drop support for ktlint-enable / ktlint-disable directive #1947

Closed
paul-dingemans opened this issue Apr 16, 2023 · 1 comment · Fixed by #2068
Closed

Drop support for ktlint-enable / ktlint-disable directive #1947

paul-dingemans opened this issue Apr 16, 2023 · 1 comment · Fixed by #2068
Milestone

Comments

@paul-dingemans
Copy link
Collaborator

The directives below can be used to suppress ktlint rules:

val foo = "foo" // ktlint-disable

/* ktlint-disable */
val foo = "foo"
/* ktlint-enable */

The usage of Suppress and SuppressWarning annotation however is more native and should be the only way starting from 1.0 version:

@Suppress("ktlint")
val foo = "foo"

Ideally, a rule is added which helps in autocorrecting the old directives to annotations. If this is implemented, take care that this rule itself should not be suppressed even in case for a line, block or file all annotations are suppressed.

@paul-dingemans paul-dingemans added this to the 1.0 (Yeah!) milestone Apr 16, 2023
@paul-dingemans
Copy link
Collaborator Author

Using Github Code Search some insight can be collected in how the old directives are used. Unfortunately, the total number of search result is currently limited to 100 results. By excluding based on repository, some more insights can be collected but this is also limited. In summer 2023 a new search API is expected which should be able to return all search results.

Insights sofar:

If a single construct is surrounded by a block directive, it should be replaced by an annotation on that single construct:

        /* ktlint-disable max-line-length */
        private const val URI_REGEX = "^(?:(?![^:@]+:[^:@/]*@)([^:/?#.]+):)?(?://)?((?:(([^:@]*)(?::([^:@]*))?)?@)?([^:/?#]*)(?::(\\d*))?)(((/(?:[^?#](?![^?#/]*\\.[^?#/.]+(?:[?#]|$)))*/?)?([^?#/]*))(?:\\?([^#]*))?(?:#(.*))?)"

        /* ktlint-enable max-line-length */

However if the directive block contains multiple statements, it can not be simply replaced with a Suppress on the first (nor on all statements). Moving the Suppress to an outer construct is not always equivalent to current situation:

    @Test
    fun testParseStringForHybridFileParcelable() {
        /* ktlint-disable max-line-length */
        // ls
        val lsLines = arrayOf(
            "-rwxr-x---   1 root   shell    29431 2009-01-01 08:00 init.rc",
            "lrw-r--r--   1 root   root        15 2009-01-01 08:00 product -> /system/product",
            "drwxr-xr-x  17 root   root      4096 1970-05-19 08:40 system",
            "-r--r--r-- 1 root root 10 1970-01-13 07:32 cpu_variant:arm",
            "lrwxrwxrwx  1 root root 0 2022-10-05 15:39 ac -> ../../devices/platform/GFSH0001:00/power_supply/ac",
            "lrwxrwxrwx   1 root root 0 2022-10-05 00:16 usb -> ../../devices/platform/soc/c440000.qcom,spmi/spmi-0/spmi0-02/c440000.qcom,spmi:qcom,pm8150b@2:qcom,qpnp-smb5/power_supply/usb"
        )

        // stat with old toybox or busybox
        // val a1 = "-rwxr-x--- 1 shell root 512 2009-01-01 08:00:00.000000000 `init.rc'"
        // val b1 = "lrw-r--r-- 1 root root 512 2009-01-01 08:00:00.000000000 `product' -> `/system/product'"
        // val c1 = "drwxr-xr-x 17 root root 512 1970-05-19 08:40:27.269999949 `system'"

        // stat with new toybox
        val statLines = arrayOf(
            "-rwxr-x--- 1 shell root 512 1230796800 `init.rc'",
            "lrw-r--r-- 1 root root 512 1230796800 `product' -> `/system/product'",
            "drwxr-xr-x 17 root root 512 11922027 `system'",
            "-r--r--r-- 1 root root 512 1035141 `cpu_variant:arm'",
            "lrwxrwxrwx 1 root root 512 1664955558 /sys/class/power_supply/ac -> '../../devices/platform/GFSH0001:00/power_supply/ac'",
            "lrwxrwxrwx 1 root root 512 1664956626 /sys/class/power_supply/usb -> '../../devices/platform/soc/c440000.qcom,spmi/spmi-0/spmi0-02/c440000.qcom,spmi:qcom,pm8150b@2:qcom,qpnp-smb5/power_supply/usb'"
        )
        /* ktlint-enable max-line-length */

        ....
    }

In example below, the block directive should be replaced with a file-annotation or the annotation must be interpreted as being relevant for the entire import list (but what if the annotation is not placed just before the first import?):

/* ktlint-disable import-ordering */
import android.app.Notification

In case below, the same EOL directive is repeated multiple times, but not on all lines. Usage of the block directive would have been a better choice in this example. But this is similar to one of problems mentioned before:

  val result = CharArray(16)
  result[ 0] = HEX_DIGIT_CHARS[(this shr 60 and 0xf).toInt()] // ktlint-disable no-multi-spaces
  result[ 1] = HEX_DIGIT_CHARS[(this shr 56 and 0xf).toInt()] // ktlint-disable no-multi-spaces
  result[ 2] = HEX_DIGIT_CHARS[(this shr 52 and 0xf).toInt()] // ktlint-disable no-multi-spaces
  result[ 3] = HEX_DIGIT_CHARS[(this shr 48 and 0xf).toInt()] // ktlint-disable no-multi-spaces
  result[ 4] = HEX_DIGIT_CHARS[(this shr 44 and 0xf).toInt()] // ktlint-disable no-multi-spaces
  result[ 5] = HEX_DIGIT_CHARS[(this shr 40 and 0xf).toInt()] // ktlint-disable no-multi-spaces
  result[ 6] = HEX_DIGIT_CHARS[(this shr 36 and 0xf).toInt()] // ktlint-disable no-multi-spaces
  result[ 7] = HEX_DIGIT_CHARS[(this shr 32 and 0xf).toInt()] // ktlint-disable no-multi-spaces
  result[ 8] = HEX_DIGIT_CHARS[(this shr 28 and 0xf).toInt()] // ktlint-disable no-multi-spaces
  result[ 9] = HEX_DIGIT_CHARS[(this shr 24 and 0xf).toInt()] // ktlint-disable no-multi-spaces
  result[10] = HEX_DIGIT_CHARS[(this shr 20 and 0xf).toInt()]
  result[11] = HEX_DIGIT_CHARS[(this shr 16 and 0xf).toInt()]
  result[12] = HEX_DIGIT_CHARS[(this shr 12 and 0xf).toInt()]
  result[13] = HEX_DIGIT_CHARS[(this shr 8  and 0xf).toInt()] // ktlint-disable no-multi-spaces
  result[14] = HEX_DIGIT_CHARS[(this shr 4  and 0xf).toInt()] // ktlint-disable no-multi-spaces
  result[15] = HEX_DIGIT_CHARS[(this        and 0xf).toInt()] // ktlint-disable no-multi-spaces

References to old experimental rule set should be replaced with standard:

        /* ktlint-disable experimental:argument-list-wrapping */
        fixture.getSut(replacements = mapOf(Replacement.FileInputStream.STRING))

In example below, the block directive should be replace with a file annotation:

/* ktlint-disable */
@file:Suppress("PackageName", "TopLevelName")

paul-dingemans added a commit that referenced this issue Jun 5, 2023
 or @SuppressWarnings annotations

Note that the SuppressionLocatorBuilder never ignore this rule as otherwise the suppression hint to ignore *all* rules would also ignore the rule which has to migrate this suppression hint.

Closes #1947
@paul-dingemans paul-dingemans modified the milestones: 1.0 (Yeah!), 0.50.0 Jun 13, 2023
paul-dingemans added a commit that referenced this issue Jun 27, 2023
* Add rule to migrate ktlint-disable directives from comments to @Suppress or @SuppressWarnings annotations

Note that the SuppressionLocatorBuilder never ignore this rule as otherwise the suppression hint to ignore *all* rules would also ignore the rule which has to migrate this suppression hint.

Closes #1947

* Fix max line length for markdown files

* Fix wildcard imports settings

* Improve trace logging by printing a lint error directly when it is emitted

The reports print the lint violations in which they occur in the source code. This is not necessarily the order in which they are detected and fixed. For debugging some type of problems, the latter order is important.

* Add "libs.logback" as runtime only dependency so that logging is visible in unit tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant