pinterest / ktlint

An anti-bikeshedding Kotlin linter with built-in formatter
https://pinterest.github.io/ktlint/
MIT License
6.06k stars 503 forks source link

v1.3.0 class-signature error for android_studio config #2713

Open keapear opened 4 days ago

keapear commented 4 days ago

Expected Behavior

According to https://pinterest.github.io/ktlint/latest/rules/standard/#class-signature, both

class Foo3(
    a: Any,
    b: Any,
)

class Foo4(a: Any, b: Any)

should be accepted when ktlint_code_style is set to android_studio

Observed Behavior

My code style is set to android_studio. After updating to v1.3.0, all cases where we use formatting like the Foo3 example (parameters not on one line even though they would fit) are considered errors.

Steps to Reproduce

Copy

class Foo3(
    a: Any,
    b: Any,
)

to a project with code style android_studio and run ktlint.

paul-dingemans commented 3 days ago

Can you confirm that you upgraded from a 0.x release of ktlint?

The class-signature rule was added in ktlint 1.0. It has never supported the behavior of allowing both signatures below:

class Foo3(
    a: Any,
    b: Any,
)

class Foo4(a: Any, b: Any)

I have no clue how this ended up in the documentation as it violates the idea behind the rule that class signature are formatted consistently. The signature above are comparable, and as of that have to be formatted in the same way.

I will update the documentation so that it is consistent with the code.

Btw you can force the multiline code style of class Foo3 by setting ktlint_class_signature_rule_force_multiline_when_parameter_count_greater_or_equal_than to 1,

keapear commented 3 days ago

No, our current ktlint version is 1.2.1. We do have a bit of a mix of class signatures in our 5 year old code base, but until now ktlint never complained.

paul-dingemans commented 3 days ago

Please provide concrete examples and their respective .editorconfig configuration so that I can check what the problem is. Please double check to run your example both with ktlint 1.2.1 and 1.3.0 with same .editorconfig.

I have no recollection of changing anything in the class-signature rule in the 1.3 release that could be related to this. Without stable reproduction I cannot help.

keapear commented 3 days ago
open class SecureStringPreference(
    private val sharedPreferences: SharedPreferences,
    private val key: String
) : BasicPreference<String>(sharedPreferences, key, ""), KoinComponent {

    override fun set(value: String) =
        sharedPreferences.edit {
            putString(key, value)
        }

    override fun get(): String {
        with(sharedPreferences) {
            return getString(key, "") as String
        }
    }
}

For this class, I get no errors with 1.2.1. With 1.3.0 I get the following:

SecureStringPreference.kt:19:5: No whitespace expected between opening parenthesis and first parameter name (standard:class-signature) SecureStringPreference.kt:20:5: Single whitespace expected before parameter (standard:class-signature) SecureStringPreference.kt:20:28: No whitespace expected between last parameter and closing parenthesis (standard:class-signature) SecureStringPreference.kt:21:58: Super type should start on a newline (standard:class-signature)

.editorconfig is the same in both cases:

# https://editorconfig.org

[*.{kt,kts}]
ktlint_code_style = android_studio
max_line_length = 120
indent_size = 4

ktlint_standard_import-ordering = disabled
ktlint_standard_trailing-comma-on-declaration-site = disabled
ktlint_standard_trailing-comma-on-call-site = disabled
ktlint_standard_indent = disabled
ktlint_standard_function-signature = disabled
ktlint_standard_statement-wrapping = disabled

# Remove after updating to ktlint 1.2. Same line comments in lists will be allowed again then
ktlint_standard_value-argument-comment = disabled

ktlint_function_naming_ignore_when_annotated_with=Composable

# Don't allow any wildcard imports
ij_kotlin_packages_to_use_import_on_demand = unset

# Prevent wildcard imports
ij_kotlin_name_count_to_use_star_import = 99
ij_kotlin_name_count_to_use_star_import_for_members = 99

[*{Test,Robot,Mock}*.{kt,kts}]
ktlint_standard_max-line-length = disabled
ktlint_standard_binary-expression-wrapping = disabled
ktlint_standard_property-wrapping = disabled
ktlint_standard_wrapping = disabled
ktlint_standard_argument-list-wrapping = disabled
ktlint_standard_parameter-list-wrapping = disabled
ktlint_standard_function-literal = disabled

ktlint_function_naming_ignore_when_annotated_with=Test
paul-dingemans commented 3 days ago

As documented in https://github.com/pinterest/ktlint/releases/tag/1.3.0 the class-signature rule has been promoted from experimental to non-experimental. Your .editorconfig has not enabled the experimental rules. As of that the rule was not run with 1.2.1 version. Now with the 1.3.0 version the rule is run by default.

You can force the multiline code style of class Foo3 by setting following in .editorconfig:

ktlint_class_signature_rule_force_multiline_when_parameter_count_greater_or_equal_than = 1

I noted that your .editorconfig does not contain root = true as first line. This can impact performance as well as it might lead to pick up .editorconfig files which is stored in a parent directory of your project. That might not be what you want.

Also, I see ktlint_function_naming_ignore_when_annotated_with=Test for [*{Test,Robot,Mock}*.{kt,kts}]. May I enquire what your use case is for this?

keapear commented 2 days ago

Alright, that makes sense, thank you. I'll add the root = true.

We have a naming convention to start our instrumentation tests with an uppercase letter + number code (e.g. E0123) to make it easier to identify them across platforms and tester documentation. It didn't really seem worth renaming them all when the ktlint rule was added.

paul-dingemans commented 1 day ago

We have a naming convention to start our instrumentation tests with an uppercase letter + number code (e.g. E0123) to make it easier to identify them across platforms and tester documentation. It didn't really seem worth renaming them all when the ktlint rule was added.

I see. It is an inventive usage for ktlint_function_naming_ignore_when_annotated_with=Test.