Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Trailing commas in enums #1542

Merged
merged 6 commits into from
Aug 3, 2022
Merged

Conversation

Kantis
Copy link
Contributor

@Kantis Kantis commented Jul 12, 2022

Description

I noticed trailing commas in enums seems to be unhandled by the trialing-comma rule. Added tests to verify that it is unhandled at the moment. I would be up for trying to add handling for enums as well, if you agree that it should be handled.

Checklist

  • PR description added
  • tests are added
  • CHANGELOG.md is updated

@paul-dingemans
Copy link
Collaborator

It is okay to add a trailing comma in enumerations although this is not yet handled correctly by IntelliJ IDEA default formatter (for example see https://youtrack.jetbrains.com/issue/KTIJ-19916/Enabledisable-trailing-comma-in-the-formatter-intention-is-not-provided-when-trailing-comma-is-used-in-enumerations). The reason that it is okay to add in Ktlint is that IntelliJ does accept enums with or without trailing comma's regardless of the settings. So implementing it correctly in Ktlint will not lead to a conflict with de default IntelliJ IDEA formatter.

@paul-dingemans
Copy link
Collaborator

Please mind handling of enum which also contain addition declarations.

Add/remove trailing commain case below depending on setting:

enum class SomeEnum1(id: String) {
    FOO("foo"),
    BAR("bar"),
    ;
    
    fun doSomething(id: String) {}
}

Note that the ; might need to be wrapped to a separate line in case the BAR item in the original code was followed by ; on the same line.

enum class SomeEnum1(id: String) {
    FOO("foo"),
    BAR("bar"), // some EOL comment
    ;
    
    fun doSomething(id: String) {}
}
enum class SomeEnum1(id: String) {
    FOO("foo"),
    BAR("bar") /* some block comment */,
    ;
    
    fun doSomething(id: String) {}
}

Finnally do not add a trailing comma in case the one-but-last and the last enumeration value are on the same line.

Having issues with PSI not containing the semicolon element
Copy link
Collaborator

@paul-dingemans paul-dingemans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution. Please address the issues.

@paul-dingemans
Copy link
Collaborator

Hi @Kantis,

Are you making any progress on this issue and are you able to finish in a couple of days? This PR is blocking #1555 (ready to be merged to master) and as a consequence also #1557.

@Kantis
Copy link
Contributor Author

Kantis commented Aug 2, 2022

Hi @Kantis,

Are you making any progress on this issue and are you able to finish in a couple of days? This PR is blocking #1555 (ready to be merged to master) and as a consequence also #1557.

Sorry for blocking. I've been zoned out a bit from coding, but I'll put in some effort to get this done to unblock the other things.

@Kantis Kantis force-pushed the trailing-comma-enum branch from 29baa22 to 305f9d6 Compare August 2, 2022 21:36
@Kantis Kantis marked this pull request as ready for review August 2, 2022 21:41
Copy link
Collaborator

@paul-dingemans paul-dingemans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code is looking good. I have simplied the code a bit and made it more consistent with other parts of the code base.

@paul-dingemans paul-dingemans added this to the 0.47.0 milestone Aug 3, 2022
@paul-dingemans paul-dingemans merged commit 7bf39e5 into pinterest:master Aug 3, 2022
@paul-dingemans paul-dingemans mentioned this pull request Aug 3, 2022
5 tasks
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants