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

standard:function-naming on composables #2259

Closed
PaulWoitaschek opened this issue Sep 12, 2023 · 13 comments · Fixed by #2275
Closed

standard:function-naming on composables #2259

PaulWoitaschek opened this issue Sep 12, 2023 · 13 comments · Fixed by #2275

Comments

@PaulWoitaschek
Copy link
Contributor

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

I found this ticket #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
Copy link
Collaborator

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.

@paul-dingemans paul-dingemans closed this as not planned Won't fix, can't repro, duplicate, stale Sep 13, 2023
@eygraber
Copy link
Contributor

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

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 added a commit that referenced this issue Sep 23, 2023
…tated_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](https://mrmans0n.github.io/compose-rules/ktlint/) can be used for checking naming conventions for such Composable functions.

Closes #2259
@paul-dingemans
Copy link
Collaborator

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.

paul-dingemans added a commit that referenced this issue Sep 23, 2023
Add `.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](https://mrmans0n.github.io/compose-rules/ktlint/) can be used for checking naming conventions for such Composable functions.

Closes #2259
@PaulWoitaschek
Copy link
Contributor Author

Nice! Thanks for the implementation 👌🎉

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

@eygraber
Copy link
Contributor

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

@pvegh
Copy link

pvegh commented Sep 25, 2023

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

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

pvegh commented Sep 25, 2023

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

@nzeugaa
Copy link

nzeugaa commented Oct 10, 2023

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

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

@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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants