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

Rule to mandate/forbid trailing commas #709

Closed
3flex opened this issue Mar 4, 2020 · 41 comments · Fixed by #1032
Closed

Rule to mandate/forbid trailing commas #709

3flex opened this issue Mar 4, 2020 · 41 comments · Fixed by #1032
Labels
Milestone

Comments

@3flex
Copy link
Contributor

3flex commented Mar 4, 2020

Expected Behavior

ktlint offers a rule that enforces a particular code style regarding trailing commas, which are now permitted by the Kotlin compiler as of 1.3.70, see https://youtrack.jetbrains.com/issue/KT-34743 for more details.

This may not be a good fit for ktlint as it's too opinionated and no conventions exist yet, but if it's suitable it's a better fit here than in detekt where it was originally proposed.

Observed Behavior

No such rule exists.

Steps to Reproduce

N/A, new rule required.

Your Environment

  • Version of ktlint used:
  • Name and version (or code for custom task) of integration used (Gradle plugin, Maven plugin, command line, custom Gradle task):
  • Version of Gradle used (if applicable):
  • Operating System and version:
  • Link to your project (if it's a public repository):
@alexvanyo
Copy link

alexvanyo commented Mar 16, 2020

Ideally this rule could be as flexible as something like ESLint's comma dangling rule, which would allow specifying when trailing commas must be used (never, always, multiline-only, when entry, etc.)

Edit: Perhaps supporting that feature set would better live elsewhere?

@Tapchicoma
Copy link
Collaborator

Probably related to #701 to control enable/disable state for cases, though initial rule implementation could just enforce trailing commas and be in experimental set.

@s1monw1
Copy link

s1monw1 commented Nov 24, 2020

Any update?

@ivorsmorenburg
Copy link

Is breaking so many things this ,
https://stackoverflow.com/questions/65055798/kotlin-linting-rulesetprovider-trailing-comma

@3flex
Copy link
Contributor Author

3flex commented Nov 30, 2020

Is this on ktlint's roadmap? If not, we could implement in detekt instead. I don't think it makes sense to do that though if ktlint will implement the rule so we can avoid duplication of effort, but I think there's demand for it (I'm sure this is my most 👍 issue ever!)

@ivorsmorenburg
Copy link

Hopefully is or it will be 🤞, for now I'm trying to implement my own rule from the link above. I can confirm there is a high demand. Will be nice to autocorrect with ktlint...

@romtsn
Copy link
Collaborator

romtsn commented Nov 30, 2020

@3flex you are more than welcome to contribute this to ktlint directly and utilize it in detekt afterwards 👀 it is certainly on the roadmap, just the maintainers were sidetracked by the day jobs and we had other priorities in ktlint, but I see this one indeed has a high demand so we'll re-prioritize

@CDehning
Copy link

Is this is getting released soon?

@ivorsmorenburg
Copy link

ivorsmorenburg commented Apr 9, 2021

Is this is getting released soon?

Maybe try using your own ruleset
https://stackoverflow.com/a/65449838/3438335

@Badneighbour91
Copy link

Badneighbour91 commented Apr 9, 2021 via email

@snijsure
Copy link

snijsure commented May 7, 2021

Is this is getting released soon?

Maybe try using your own ruleset
https://stackoverflow.com/a/65449838/3438335

Is there a guideline on integrating this rule into local version of local ktlint?

@christian-draeger
Copy link

any news?

@TobiasMende
Copy link

TobiasMende commented Jun 10, 2021

We are currently using our own rule for mandating (and automatically fixing) missing trailing commas. Probably it is not ready yet to be added to the ktlint project, but I am happy to have a discussion about it:

class MandateTrailingComma : Rule("mandate-trailing-comma") {

    override fun visit(
        node: ASTNode,
        autoCorrect: Boolean,
        emit: (offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit,
    ) {
        if (node.isArgumentOrParameterList() && node.isMultiLine()) {
            val lastArgument = node.lastArgumentOrParameter()
            if (lastArgument != null && lastArgument.treeNext.elementType != COMMA) {
                emit(
                    lastArgument.startOffset + lastArgument.textLength,
                    "Trailing comma is missing",
                    true,
                )
                if (autoCorrect) {
                    node.addChild(LeafPsiElement(COMMA, ","), lastArgument.treeNext)
                }
            }
        }
    }

    private fun ASTNode.isArgumentOrParameterList(): Boolean =
        elementType == VALUE_ARGUMENT_LIST || elementType == VALUE_PARAMETER_LIST

    private fun ASTNode.isMultiLine(): Boolean = children().any { it is PsiWhiteSpace && it.textContains('\n') }
    private fun ASTNode.lastArgumentOrParameter() =
        children().findLast { it.elementType == VALUE_ARGUMENT || it.elementType == VALUE_PARAMETER }
}

romtsn pushed a commit to paul-dingemans/ktlint that referenced this issue Aug 8, 2021
romtsn pushed a commit to paul-dingemans/ktlint that referenced this issue Aug 8, 2021
romtsn pushed a commit to paul-dingemans/ktlint that referenced this issue Aug 8, 2021
@sindrenm
Copy link

sindrenm commented Aug 9, 2021

Fantastic stuff! Just to be clear, “allowing” trailing commas here means requiring trailing commas, right? Will

fun foo(
  param1: Bar,
  param2: Bar
)

be converted into

fun foo(
  param1: Bar,
  param2: Bar, // <-- added trailing comma
)

when either running ktlint -F or the auto-formatter in IJ/AS?

@romtsn
Copy link
Collaborator

romtsn commented Aug 9, 2021

Yes, this means requiring, that's correct (and that's what IJ does as well, when ticking the "Allow trailing comma" checkbox in settings). I will update README with a better description of the behavior 👍

@snijsure
Copy link

I might be doing something wrong, and I don't see ktlint -F is putting missing trailing commas,

Here is my setup:

I have the following in .editorconfig

ij_kotlin_allow_trailing_comma = true
ij_kotlin_allow_trailing_comma_on_call_site = true

And I have code like this

    data class State(
        val tutorialPages: List<TutorialPage> = emptyList(),
        val currentPageNumber: Int = 0
    ) 

When I run command

ktlint -F --editorconfig=.editorconfig --experimental fileName

I see the following error

WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by org.jetbrains.kotlin.com.intellij.util.ReflectionUtil (file:/usr/local/Cellar/ktlint/0.42.1/libexec/ktlint) to field java.lang.Throwable.backtrace
WARNING: Please consider reporting this to the maintainers of org.jetbrains.kotlin.com.intellij.util.ReflectionUtil
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release

I don't see that ktlint -F has put the trailing comma.

  data class State(
        val tutorialPages: List<TutorialPage> = emptyList(),
        val currentPageNumber: Int = 0**,**
    ) 

@romtsn
Copy link
Collaborator

romtsn commented Aug 12, 2021

@snijsure which version are you using? ktlint --version

@snijsure
Copy link

@romtsn I am using version 0.42.1

ktlint --version
0.42.1

@romtsn
Copy link
Collaborator

romtsn commented Aug 12, 2021

yeah, as I mentioned it's only available in the snapshot version for now.

@Lingviston
Copy link

When is it going to be live?

@zsmb13
Copy link

zsmb13 commented Nov 2, 2021

A new release containing this was just created https://github.com/pinterest/ktlint/releases/tag/0.43.0

@mohsenoid
Copy link

mohsenoid commented Dec 1, 2021

I am using 0.43.0 CLI
image
and enabled the following in my project's .editorconfig

ij_kotlin_allow_trailing_comma = true
ij_kotlin_allow_trailing_comma_on_call_site = true

but I don't get any format fix or lint error from ktlint when I execute this or related Gradle tasks:
ktlint -F "app/src/**/*.kt"

The good news is, the Android studio code styler understands it and does the formatting for me, but I also want it to be a part of the CI check!

@romtsn
Copy link
Collaborator

romtsn commented Dec 1, 2021

@mohsenoid

Note, that it's available as a --experimental rule for now.

@snijsure
Copy link

snijsure commented Dec 1, 2021

Is there a way to just enable this rule, out of the experimental ruleset?

If I turn on experimental rules it turns up lots of other warnings that we don't care at this moment

@romtsn

@mohsenoid
Copy link

mohsenoid commented Dec 1, 2021

@romtsn Thanks for the hint, I executed this and it works fine:
ktlint -F --experimental "app/src/**/*.kt"

Looking forward to seeing the final rule.

@romtsn
Copy link
Collaborator

romtsn commented Dec 1, 2021

@snijsure nope, the best you can do is to enable the experimental ruleset and disable the rules you don't care about via disabled_rules option

@mohsenoid
Copy link

Since this is an experimental rule, should we report issues here or create a new ticket case by case?

For instance in this case
https://github.com/mohsenoid/Rick-and-Morty/blob/c9fb8dc093c7986055e8892b8e126f60bf0247cc/app/src/main/kotlin/com/mohsenoid/rickandmorty/data/db/DbRickAndMorty.kt#L51

Android Studio works correctly and adds a comma, but Ktlint doesn't and even removes the comma added by Android Studio!

@romtsn
Copy link
Collaborator

romtsn commented Dec 1, 2021

@mohsenoid please file a new issue

@vojkny
Copy link

vojkny commented Dec 11, 2021

how can I enable this rule via .editorconfig? I used:

ij_kotlin_allow_trailing_comma = true
ij_kotlin_allow_trailing_comma_on_call_site = true

but seems to have no effect

@romtsn
Copy link
Collaborator

romtsn commented Dec 11, 2021

#709 (comment)

@vojkny
Copy link

vojkny commented Dec 11, 2021

#709 (comment)

It does not answer how I can enable it via .editorconfig.

@romtsn
Copy link
Collaborator

romtsn commented Dec 11, 2021

well, your question was about the trailing comma rule, not about the experimental ruleset. you can't enable experimental via editorconfig

@Egorand
Copy link

Egorand commented Jan 22, 2022

Can this feature be used with spotless?

@sindrenm
Copy link

Can this feature be used with spotless?

Not as far as I can tell, since experimental ktlint flags can't be enabled. (diffplug/spotless#409)

@krisgerhard
Copy link

This feature seems to break no-unused-imports rule. I'm getting "unused import" error for all imports that are used on trailing comma lines exclusively

@hongjisung
Copy link

I have same unused-imports rule break error with -F --experimental option

I run ktlint -F --experimental three times then the project pass the test and fixed correctly
But, first and second trials are failed with only some modified files

@krisgerhard
Copy link

I created a failing test-case and new issue: #1367

@mikaello
Copy link

mikaello commented Jul 18, 2022

In Ktlint v0.46.0, trailing-comma was promoted from the experimental ruleset to the standard ruleset. Adding this to your .editorconfig should now work without having to specify --experimental flag when running Ktlint:

[*.{kt,kts}]
ij_kotlin_allow_trailing_comma_on_call_site=true
ij_kotlin_allow_trailing_comma=true

If you use the ktlint-maven-plugin, you must use v1.14.0 or later, example:

$ mvn com.github.gantsign.maven:ktlint-maven-plugin:1.14.0:format

@bgolob
Copy link

bgolob commented Sep 15, 2022

In Ktlint v0.46.0, trailing-comma was promoted from the experimental ruleset to the standard ruleset. Adding this to your .editorconfig should now work without having to specify --experimental flag when running Ktlint:

[*.{kt,kts}]
ij_kotlin_allow_trailing_comma_on_call_site=true
ij_kotlin_allow_trailing_comma=true

If you use the ktlint-maven-plugin, you must use v1.14.0 or later, example:

$ mvn com.github.gantsign.maven:ktlint-maven-plugin:1.14.0:format

I enabled both of the settings for trailing commas. I'm using Ktlint Gradle plugin version 11.0.0. When I run ./gradlew ktlintcheck
I'm not getting any errors for missing trailing commas.

For example here the trailing comma is missing and ktlint didn't throw any errors

data class AdModel(
    val duration: Int,
    val type: String,
    val url: String
)

@mikaello
Copy link

mikaello commented Sep 15, 2022

I enabled both of the settings for trailing commas. I'm using Ktlint Gradle plugin version 11.0.0. When I run ./gradlew ktlintcheck
I'm not getting any errors for missing trailing commas.

I am not familiar with Ktlint Gradle plugin, but it seems that the newest version of that plugin, v11.0.0, is using Ktlint v0.43.2. Between Ktlint [v0.43.0, v0.46.0), the trailing comma rule was only present in the experimental ruleset.

Skimming trough the issue list of Ktlint Gradle plugin it is at least two issues for upgrading Ktlint, and problems with that, e.g.:

You should follow those issue to get notified when Ktlint Gradle plugin is compatible with newer versions of Ktlint.

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