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

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

Closed
keapear opened this issue Jun 24, 2024 · 7 comments · Fixed by #2729
Closed

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

keapear opened this issue Jun 24, 2024 · 7 comments · Fixed by #2729

Comments

@keapear
Copy link

keapear commented Jun 24, 2024

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
Copy link
Collaborator

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
Copy link
Author

keapear commented Jun 25, 2024

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
Copy link
Collaborator

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
Copy link
Author

keapear commented Jun 25, 2024

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
Copy link
Collaborator

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
Copy link
Author

keapear commented Jun 26, 2024

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
Copy link
Collaborator

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.

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.

2 participants