saveourtool / diktat

Strict coding standard for Kotlin and a custom set of rules for detecting code smells, code style issues and bugs
https://diktat.saveourtool.com
MIT License
516 stars 38 forks source link

Wrong behavior for `ENUMS_SEPARATED` and `ENUM_VALUE` rule #1319

Open WebTiger89 opened 2 years ago

WebTiger89 commented 2 years ago

[Kotlin] I have the following enum class:

enum class DeviceType(val value: Int, val humanReadableName: String) {
    SDR(1, "SDR"),
    UCM(2, "UCM"),
    UCM_CARD_READER(3, "UCM-Cardreader"),
    DSD_AM(255, "DSD");

    companion object { ... }

The rule wants to do this (check last entry):

enum class DeviceType(val value: Int, val humanReadableName: String) {
    SDR(1, "SDR"),
    UCM(2, "UCM"),
    UCM_CARD_READER(3, "UCM-Cardreader"),
    DSD_AM(255, "DSD"),
    ;

    companion object { ... }

I get 2 warnings for this enum class:

[ENUMS_SEPARATED] enum is incorrectly formatted: semicolon must be on a new line

[ENUMS_SEPARATED] enum is incorrectly formatted: last enum entry must end with a comma

I'm not sure about this rule because unfortunately, Kotlin style guide does not mention this case: https://kotlinlang.org/docs/coding-conventions.html#property-names

and I have never seen this style before.

Also:

[ENUM_VALUE] enum values should be in selected UPPER_CASE snake/PascalCase format: DSD_AM

In config file is stated:

Checks that enum value is in non UPPER_SNAKE_CASE or non PascalCase depending on the config, UPPER_SNAKE_CASE by default

The part after the comma confuses me, because it implies that UPPER_SNAKE_CASE is a valid format

The enumstyle is set to snakeCase in my config file and it is the default value.

# Two options: snakeCase(default), PascalCase

enumStyle: snakeCase

Also a bit confusing because snakeCase = myEnumEntry? I have never seen this format before on enum entries.

orchestr7 commented 2 years ago

Speaking about ENUMS - we describe the logic and motivation here: https://github.com/saveourtool/diktat/blob/master/info/guide/diktat-coding-convention.md#-39-enumerations

Our huge experience in big enterprise teams shows that it is very important to use trailing comma and a semicolon on a separate line because of one main thing: conflicts during the review. This is why this feature (trailing comma) was added to late Kotlin.

orchestr7 commented 2 years ago

Speaking about the documentation - it looks like it is really confusing. This inspection expects you to write enum values in UPPER_SNAKE_CASE. Yes, we definitely must update documentation.

WebTiger89 commented 2 years ago

Thanks for the explanation. Can you please explain what the rule expects when snakeCase is set and what is expected when PascalCase is set? When I got you right I would say: snakeCase = MY_ENUM_ENTRY PascalCase = MyEnumEntry

is that right?

orchestr7 commented 2 years ago

snakeCase = MY_ENUM_ENTRY PascalCase = MyEnumEntry

These are notations:

camelCase
PascalCase
lower_snake_case
UPPER_SNAKE_CASE

By the default we expect enums in UPPER_SNAKE_CASE (MY_ENUM_ENTRY). But you can set your style - you can select PascalCase in the rule and in this case we won't trigger an error on enums in this case set.

So in your code enums are in the valid notation for the diktat (in UPPER_SNAKE_CASE):

    SDR(1, "SDR"),
    UCM(2, "UCM"),
    UCM_CARD_READER(3, "UCM-Cardreader"),
    DSD_AM(255, "DSD"),
orchestr7 commented 2 years ago

I will update documentation in the rule.

Two options: snakeCase(default), PascalCase

That is a bug in documentation - author wanted to say UPPER_SNAKE_CASE instead of snakeCase

WebTiger89 commented 2 years ago

ok thanks. Then you might also want to adjust the documentation line which mentions only two options where snakeCase is wrong and the first line:

'#Checks that enum value is in non UPPER_SNAKE_CASE or non PascalCase depending on the config, UPPER_SNAKE_CASE by default name: ENUM_VALUE enabled: true configuration:

Two options: snakeCase(default), PascalCase

enumStyle: snakeCase

WebTiger89 commented 2 years ago

We have the same thoughts :D

orchestr7 commented 2 years ago

We have the same thoughts :D

Welcome to contribute to our project with a such mindset :D

WebTiger89 commented 2 years ago

@akuleshov7 Offtopic: How does contribution work. I could improve the documentation when I have time.

I have noticed something else. In the diktat style guide is mentioned:

If the enum is simple (no properties, methods, and comments inside), you can declare it in a single line: enum class Suit { CLUBS, HEARTS, SPADES, DIAMONDS }

But with default configuration I get warnings for

[BRACES_BLOCK_STRUCTURE_ERROR] braces should follow 1TBS style: incorrect same line after opening brace

on this enum class:

enum class ShimmerAnimationType { FADE, TRANSLATE, FADE_TRANSLATE, VERTICAL }

so BRACES_BLOCK_STRUCTURE_ERROR should be adjusted to work with enum classes.

And the other warning:

[ENUM_VALUE] enum values should be in selected UPPER_CASE snake/PascalCase format: MK64FN1M0xxx12

on this enum class:

enum class ProcessorType(val value: Int, val humanReadableName: String) {
    M16C62A(0, "M16C62A"),
    M16C62P(1, "M16C62P"),
    M16C63(2, "M16C63"),
    MK64FN1M0xxx12(16, "MK64FN1M0xxx12"),
    K64FX512xxx12(17, "K64FX512xxx12"),
    ;

Basically it is UPPER_SNAKE_CASE, it's just one long word, quasi the UPPER of UPPER_SNAKE_CASE. Do you see a problem in the naming of the entries or might there be some room for improvement for this rule to recognize this as UPPER_SNAKE_CASE?

Config:

- name: ENUM_VALUE
  enabled: true
  configuration:
    # Two options: snakeCase(default), PascalCase
    enumStyle: snakeCase

- name: BRACES_BLOCK_STRUCTURE_ERROR
  enabled: true
  configuration:
    openBraceNewline: 'True'
    closeBraceNewline: 'True'
orchestr7 commented 2 years ago

partially fixed in #1332

petertrr commented 2 years ago

Offtopic: How does contribution work. I could improve the documentation when I have time.

We have some information in CONTRIBUTING.md, but basically you'll need to fork our repo, make changes in your fork and then create a pull request.