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

Enforce trailing commas at declaration site by default #1669

Merged
merged 5 commits into from Nov 6, 2022

Conversation

3flex
Copy link
Contributor

@3flex 3flex commented Oct 7, 2022

Description

Kotlin Coding Conventions says:

The Kotlin style guide encourages the use of trailing commas at the declaration site

But ktlint's default config for this rule enforces no comma which directly conflicts with the coding conventions which encourages trailing commas.

Checklist

  • PR description added
  • tests are added
  • KtLint has been applied on source code itself and violations are fixed
  • documentation is updated
  • CHANGELOG.md is updated

In case of adding a new rule:

@paul-dingemans
Copy link
Collaborator

Tnx for your patch. For this setting I tend to agree with you that true as default setting is more aligned with Kotlin Coding Conventions. For Android, I can not find such a recommendation and as of that it should not change (set androidDefaultValue to false). Please also update the documentation and changelog.

@3flex
Copy link
Contributor Author

3flex commented Oct 13, 2022

I don't follow the logic for Android - I thought the Android style guide was essentially considered an extension of the Kotlin Coding Conventions, with some extra rules, and a few things that override the standard conventions, though perhaps that's not how ktlint deals with it.

In any case, I don't think it's correct to set the Android value to false here: that means commas will be forbidden at declaration sites on Android which doesn't make sense - if commas aren't enforced because it's not covered by the Android style guide, then they shouldn't be forbidden, for the same reason.

If you don't agree I might make this a draft and open a new issue to discuss further and perhaps get external input.

@paul-dingemans
Copy link
Collaborator

I don't follow the logic for Android - I thought the Android style guide was essentially considered an extension of the Kotlin Coding Conventions, with some extra rules, and a few things that override the standard conventions, though perhaps that's not how ktlint deals with it.

As far as I know, the Kotlin Coding Convention and Androids Kotlin Style Guide are indenpendent of each other, but luckily they do overlap on a lot of things. For some editorconfig settings a different default value is used however.

In any case, I don't think it's correct to set the Android value to false here: that means commas will be forbidden at declaration sites on Android which doesn't make sense - if commas aren't enforced because it's not covered by the Android style guide, then they shouldn't be forbidden, for the same reason.

KtLint indeed has a more strict interpretation of the the flags than IntelliJ / Android. KtLint considers a consistent usage of the trailing more important than IntelliJ / Android. As the rules are already released with default value false, this won't change the behavior for Android style guide by setting the androidDefaultValue to false while defaultValue (for default Kotlin style) is set to true.

If you don't agree I might make this a draft and open a new issue to discuss further and perhaps get external input.

I leave this up to you. However, I do not expect much input of the community when you raise this as a separate issue or mention it in the Slack channel. I would love to have more debate with the community about such changes but I have not found a way to get sufficient and timely feedback of the community.

@3flex
Copy link
Contributor Author

3flex commented Oct 18, 2022

the Kotlin Coding Convention and Androids Kotlin Style Guide are indenpendent of each other

Android Kotlin code is still Kotlin code, so wouldn't the Kotlin Coding Conventions apply? As long as the Android style guide doesn't have a more specific rule or override the Kotlin Coding Conventions, where you'd use the more specific guidance.

If we have androidDefaultValue = false while defaultValue = true then Android projects that have Kotlin/JVM modules will require trailing commas in the JVM modules and forbid them in the Android modules which I think will be quite confusing.

I'll still proceed with the PR even if we won't make them consistent, but hopefully this is convincing enough that we can have the same default for both Android and JVM...

@paul-dingemans
Copy link
Collaborator

the Kotlin Coding Convention and Androids Kotlin Style Guide are indenpendent of each other

Android Kotlin code is still Kotlin code, so wouldn't the Kotlin Coding Conventions apply? As long as the Android style guide doesn't have a more specific rule or override the Kotlin Coding Conventions, where you'd use the more specific guidance.

From the Android Kotlint Style Guide:

This document serves as the complete definition of Google’s Android coding standards for source code in the Kotlin Programming Language. A Kotlin source file is described as being in Google Android Style if and only if it adheres to the rules herein.

To me, the word complete suggests that it is independent of any other coding standard.

If we have androidDefaultValue = false while defaultValue = true then Android projects that have Kotlin/JVM modules will require trailing commas in the JVM modules and forbid them in the Android modules which I think will be quite confusing.

This is a valid concern. But the same is already happening for the max line length as well. For Android the default value is set to 100 while for default Kotlin the max line length is not set.

@BraisGabin
Copy link

Following the arguments I would vote to disable the rule for android by default. If the code convention says nothing it doesn't imply that they should be forbidden.

But I would rather prefer to follow the Kotlin convention when the android convention mention nothing about it.

@paul-dingemans paul-dingemans merged commit 362fa8c into pinterest:master Nov 6, 2022
@3flex 3flex deleted the patch-1 branch November 6, 2022 21:16
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 this pull request may close these issues.

None yet

3 participants