pinterest / ktlint

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

standard:function-naming on composables #2259

Closed PaulWoitaschek closed 11 months ago

PaulWoitaschek commented 1 year ago

Ktlint 1.0.0 warns that composable functions should be named lower case.

Composable functions should be named CamelCase and not lowerCase.

Steps to reproduce

package yazio.debug.components

import androidx.compose.material.Text
import androidx.compose.runtime.Composable

@Composable
fun Composey() {
    Text("Hey")
}
  1. create a file called Composey.kt
  2. Download ktlint
  3. call ./ktlint Composey.kt
  4. Output:
    
    Composey.kt:7:5: Function name should start with a lowercase letter (except factory methods) and use camel case (standard:function-naming)

Summary error count (descending) by rule: standard:function-naming: 1

PaulWoitaschek commented 1 year ago

I found this ticket https://github.com/pinterest/ktlint/issues/2139

But find this not acceptable. A lot of projects use compose somewhere in their code base. I do not think that it makes sense to enable it by default in the current state.

paul-dingemans commented 1 year ago

But find this not acceptable. A lot of projects use compose somewhere in their code base. I do not think that it makes sense to enable it by default in the current state.

The wording is not acceptable is so to say somewhat strange in the context of an open source project. You are not the owner of the project. And I am not a contractor that can be told whether something is acceptable or not. I assume that it is not intended like this, but this is typically a kind of wording that has negative impact on maintainers.

Android Kotlin style guide says following about naming of Composable functions:

Functions annotated with @Composable that return Unit are PascalCased and named as nouns, as if they were types.

@Composable
fun NameTag(name: String) {
    // …
}

So in one way, you're correct to say that according to kotlin style guide it is allowed to start function names with a capital given that other conditions are also met. But this does not apply, when not using composeables. So it becomes arguable whether the function-naming rule should be enabled for code style android_studio. Ktlint is definitely not going to check the additional requirements when a function name starts with a capital (especially the checking for 'Nouns' is way out of scope).

My advice would be to disable this specific rule in your .editorconfig. If you have hard evidence that at least 80% of Android projects are using composables, I will consider to find a way to disable this rule for the android_studio code style only. Until then, I am closing the issue.

eygraber commented 12 months ago

Compose is available beyond Android (https://github.com/JetBrains/compose-multiplatform) so limiting it to the android_studio code style probably isn't a good long term solution. Is it possible to change the rules for function-naming based on the presence of a Composable annotation with a Unit return type?

paul-dingemans commented 11 months ago

Ktlint will not be tailored for specific libraries like Compose. Although I dislike adding all kinds of configuration options (bikeshedding principle), it could be considered to add .editorconfig property ktlint_function_naming_ignore_annotation_pattern. This property contains a single regexp. If the function is annotated and any annotation name matches the pattern it will be ignored by the rule.

In the FAQ of ktlint it should then be documented how Compose projects should configure ktlint by setting .editorconfig property ktlint_function_naming_ignore_annotation_pattern to value @Composable. For more detailed checking on whether the Compose rules are used, a more specific ruleset should be used (for example https://mrmans0n.github.io/compose-rules/ktlint/).

paul-dingemans commented 11 months ago

I have added .editorconfig property ktlint_function_naming_ignore_when_annotated_with.

When using Compose, set property ktlint_function_naming_ignore_when_annotated_with=Composable to suppress the function-naming rule for functions annotated with @Composable. A dedicated ktlint ruleset like Compose Rules can be used for checking naming conventions for such Composable functions.

PaulWoitaschek commented 11 months ago

Nice! Thanks for the implementation 👌🎉

For many, compose isn't just "a library", but the standard to build UI elements on Kotlin.

eygraber commented 11 months ago

Thanks, this'll go a long way for the ecosystem!

pvegh commented 11 months ago

@paul-dingemans I'm not sure if my comment will be visible to you but in the changes I've seen you referenced 1.0.1 which is a dead link right now. Did you intend to release 1.0.1 alongside this change? Update: I just saw that PR is still open, sorry.

paul-dingemans commented 11 months ago

Yes, link is still dead as the release has not been built. Please see snapshot docs https://pinterest.github.io/ktlint/dev-snapshot/rules/standard/#function-naming until it is released. See https://pinterest.github.io/ktlint/dev-snapshot/install/snapshot-build/ if you want to use the snapshot build till then.

pvegh commented 11 months ago

Yep I noticed my mistake. I'll try the snapshot.

nzeugaa commented 11 months ago

@paul-dingemans any timeline when do you intend to release 1.0.1 with this change ? we are facing this issue when using megalinter

paul-dingemans commented 11 months ago

@paul-dingemans any timeline when do you intend to release 1.0.1 with this change ? we are facing this issue when using megalinter

It is a bit hard to say at this moment. I intended to release on Oct 1st but did not have enoug time for it. And last couple of days, some new (regression) problems were reported which I want to include as well. You should not expect a release in next two weeks.

paul-dingemans commented 11 months ago

@paul-dingemans any timeline when do you intend to release 1.0.1 with this change ? we are facing this issue when using megalinter

It is a bit hard to say at this moment. I intended to release on Oct 1st but did not have enoug time for it. And last couple of days, some new (regression) problems were reported which I want to include as well. You should not expect a release in next two weeks.

Release is on its way...