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

ElseCaseInsteadOfExhaustiveWhen config to fail when there's only one unused type/value #5623

Closed
eygraber opened this issue Dec 15, 2022 · 6 comments · Fixed by #5634
Closed
Labels
good first issue Issue that is easy to pickup for people that are new to the project help wanted rules

Comments

@eygraber
Copy link
Contributor

I tried enabling ElseCaseInsteadOfExhaustiveWhen but I had way too many violations, and most of them were intentional (e.g. there's a Result type that has Success type and a bunch of failure types, and I only want to know if it was a success or failure, and don't care about the specific failure).

However, I found a few cases where I intended to use an exhaustive when, but I either missed a type, or didn't handle null, and used else instead.

I think it would be helpful if there was a config (or separate rule) that would allow an else in a when, unless there was only one more type that has to be handled (or null).

@eygraber eygraber added the rules label Dec 15, 2022
@cortinico
Copy link
Member

I think it would be helpful if there was a config (or separate rule) that would allow an else in a when, unless there was only one more type that has to be handled (or null).

I somehow disagree with this. Having a else in a when, even if there only one case less to handle, captures a wider scope of values.

The whole point about not using else is that when you edit one of those enum or sealed class, you're forced to update the whens.

Do you have some examples to share?

@eygraber
Copy link
Contributor Author

I have a bunch that all follow the same pattern; a when that checks one or more possible cases, but less than n - 1 of the possible cases.

For example using kotlinx json, I have a lot of whens that look like:

when(json) {
  is JsonObject ->
  else -> 
}

In that example an if..else is more lines, and I like the when better because for small expressions it allows the case and result to be read on the same line for easier mental parsing.

I know I could do:

when(json) {
  is JsonObject ->
  is JsonArray, is JsonPrimitive, JsonNull ->
}

but that's a lot more mental parsing now. And kotlinx json only had a few cases. What if there were 20 unhandled cases that all get handled the same way?

For example, I have a sealed class that models API errors where's there's a Success subtype, and 20 different subtypes for errors. 99% of the time I do:

when(result) {
  is Success ->
  else ->
}

Now I could (and probably should) introduce a hierarchy with a top level Error but I don't always control the code and can't dictate that.

The whole point about not using else is that when you edit one of those enum or sealed class, you're forced to update the whens.

I actually agree with this very strongly, but it doesn't always work in practice. I wish Kotlin would've added an exhaustive keyword.

@cortinico
Copy link
Member

This feels to me like either:

  • A valid scenario for a local suppressions
  • A valid scenario to introduce a ignoreSubjectTypes and have a list of fully qualified types you want this rule to be ignored for.

@eygraber
Copy link
Contributor Author

The later seems more reasonable. I have a few hundred occurrences of this, and local suppression wouldn't be an option because of that.

@cortinico
Copy link
Member

The later seems more reasonable. I have a few hundred occurrences of this, and local suppression wouldn't be an option because of that.

We're happy to receive a PR implementing this 🙏

@cortinico cortinico added help wanted good first issue Issue that is easy to pickup for people that are new to the project labels Dec 19, 2022
mmorozkov added a commit to mmorozkov/detekt that referenced this issue Dec 22, 2022
…unused type/value detekt#5623

A new `ignoredSubjectTypes` property with an empty default value was introduced. It can be configured with a list of subject types fully qualified names which should be ignored by the `ElseCaseInsteadOfExhaustiveWhen` rule.
@mmorozkov
Copy link
Contributor

Hi,
I submitted a PR to address this: #5634

mmorozkov added a commit to mmorozkov/detekt that referenced this issue Dec 22, 2022
A new `ignoredSubjectTypes` property with an empty default value was introduced. It can be configured with a list of subject types fully qualified names which should be ignored by the `ElseCaseInsteadOfExhaustiveWhen` rule.
mmorozkov added a commit to mmorozkov/detekt that referenced this issue Dec 26, 2022
A new `ignoredSubjectTypes` property with an empty default value was introduced. It can be configured with a list of subject types fully qualified names which should be ignored by the `ElseCaseInsteadOfExhaustiveWhen` rule.
mmorozkov added a commit to mmorozkov/detekt that referenced this issue Jan 19, 2023
A new `ignoredSubjectTypes` property with an empty default value was introduced. It can be configured with a list of subject types fully qualified names which should be ignored by the `ElseCaseInsteadOfExhaustiveWhen` rule.
schalkms pushed a commit that referenced this issue Feb 4, 2023
A new `ignoredSubjectTypes` property with an empty default value was introduced. It can be configured with a list of subject types fully qualified names which should be ignored by the `ElseCaseInsteadOfExhaustiveWhen` rule.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Issue that is easy to pickup for people that are new to the project help wanted rules
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants