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

Enable Ktlint two TrailingComma rules to default setting in Ktlint #5386

Closed
wants to merge 2 commits into from

Conversation

chao2zhang
Copy link
Member

@chao2zhang chao2zhang commented Oct 7, 2022

From #5312 (comment), this PR demonstrates what it looks like when enabling the two TrailingComma rules with ktlint's default setting.

  TrailingCommaOnCallSite:
    active: false
    autoCorrect: true
    useTrailingCommaOnCallSite: false
  TrailingCommaOnDeclarationSite:
    active: false
    autoCorrect: true
    useTrailingCommaOnDeclarationSite: false

My vote would be to configure these rules per Kotlin coding convention

Trailing commas are entirely optional – your code will still work without them. The Kotlin style guide encourages the use of trailing commas at the declaration site and leaves it at your discretion for the call site.

  TrailingCommaOnCallSite:
    active: false
    autoCorrect: true
    useTrailingCommaOnCallSite: false
  TrailingCommaOnDeclarationSite:
    active: true
    autoCorrect: true
    useTrailingCommaOnDeclarationSite: true

@3flex
Copy link
Member

3flex commented Oct 7, 2022

I've opened pinterest/ktlint#1669 & pinterest/ktlint#1670 to get the defaults changed for ktlint, hopefully maintainers agree. I don't think the current defaults make sense either, but also think that if we apply the ktlint rules, then we should apply the same config by default.

@chao2zhang chao2zhang closed this Nov 1, 2022
@3flex 3flex reopened this Nov 6, 2022
@3flex
Copy link
Member

3flex commented Nov 6, 2022

Changes have just been made upstream, I think it makes sense to change default config in detekt to align before 1.22.0 is released - otherwise users have to deal with the trailing commas rule split in 1.22 then a change to default config in 1.23. It's better if they can deal with both at once.

Upstream both rules were changed so commas are required for standard Kotlin and forbidden for Android. That wasn't my suggestion (I strongly believe they should be forced on for both by default) so we either override ktlint defaults here, or try and convince ktlint maintainer to reconsider.

The conversation re: Kotlin vs Android defaults is here pinterest/ktlint#1669, but now the PR is merged it's probably appropriate to open a new issue for further discussion.

@chao2zhang
Copy link
Member Author

  • I will follow the upstream for the default configurations for ktlint
  • I will also add references in the rule description as well for people to refer to the official coding convention for Kotlin and Android, before opening new issues in our repository.

@chao2zhang
Copy link
Member Author

By the way I would still like to close this PR so that the history of PR is preserved without force push.

@3flex
Copy link
Member

3flex commented Nov 7, 2022

I think force push is fine but up to you, no problem if you want to close.

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

Successfully merging this pull request may close these issues.

None yet

2 participants