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.

Sign up for GitHub

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

Jump to bottom

ElseCaseInsteadOfExhaustiveWhen - Add ignoredSubjectTypes config key (#5623) #5634

Merged
merged 1 commit into from Feb 4, 2023
Merged

ElseCaseInsteadOfExhaustiveWhen - Add ignoredSubjectTypes config key (#5623) #5634

merged 1 commit into from Feb 4, 2023

Conversation

mmorozkov
Copy link
Contributor

@mmorozkov mmorozkov commented Dec 22, 2022 •
edited by cortinico

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.

Fixes #5623

@github-actions github-actions bot added the rules label Dec 22, 2022
@mmorozkov mmorozkov mentioned this pull request Dec 22, 2022
Closed
@codecov
Copy link

codecov bot commented Dec 22, 2022 •
edited

Codecov Report

Merging #5634 (e69fc1e) into main (b780c88) will not change coverage.
The diff coverage is n/a.

❗ Current head e69fc1e differs from pull request most recent head 7b8c162. Consider uploading reports for the commit 7b8c162 to get more accurate results

@@     Coverage Diff      @@
##   main   #5634   +/-   ##
============================
============================

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@cortinico cortinico changed the title ElseCaseInsteadOfExhaustiveWhen config update (#5623) ElseCaseInsteadOfExhaustiveWhen - Add ignoredSubjectTypes config key (#5623) Dec 24, 2022
cortinico
cortinico approved these changes Dec 24, 2022
View reviewed changes
Copy link
Member

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

LGTM 👍

...ne/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/ElseCaseInsteadOfExhaustiveWhen.kt Outdated Show resolved Hide resolved
@cortinico cortinico added this to the 1.23.0 milestone Dec 24, 2022
@vercel
Copy link

vercel bot commented Dec 26, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
detekt 🔄 Building (Inspect) Dec 26, 2022 at 11:45AM (UTC)

@G00fY2
Copy link
Contributor

G00fY2 commented Jan 5, 2023

@mmorozkov I initially created this rule and I really like the addition in this PR. The new config property is especially useful for when statements with a sealed class subject that represent the success and specific error states. We have something similar in our project and currently use a lot of suppress annotations.

LGTM

@mmorozkov
Copy link
Contributor Author

@cortinico
What are the next steps? Is there anything on me that blocks this PR to be merged?

BraisGabin
BraisGabin reviewed Jan 12, 2023
View reviewed changes
...ne/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/ElseCaseInsteadOfExhaustiveWhen.kt Outdated
@@ -70,6 +79,10 @@ class ElseCaseInsteadOfExhaustiveWhen(config: Config = Config.empty) : Rule(conf
if (whenExpression.elseExpression == null) return

val subjectType = subjectExpression.getType(bindingContext)
if (ignoredSubjectTypes.contains(subjectType?.fqNameOrNull().toString())) {
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
if (ignoredSubjectTypes.contains(subjectType?.fqNameOrNull().toString())) {
if (ignoredSubjectTypes.contains(subjectType?.fqNameOrNull()?.toString())) {

I don't know what contains thinks about null. But checking for "null" doesn't seem 100% correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@BraisGabin
Copy link
Member

Could you check comment? Once that is addressed I think that this is good to go.

@mmorozkov
ElseCaseInsteadOfExhaustiveWhen config update (detekt#5623)
7b8c162 
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 Author

Could you check comment? Once that is addressed I think that this is good to go.

@BraisGabin Done!

@schalkms
Copy link
Member

@BraisGabin anything left to do there? Otherwise, we should get this merged for inclusion in the detekt next version.

@mmorozkov
Copy link
Contributor Author

@BraisGabin @schalkms @cortinico guys is there anything that block this one from being merged?

@schalkms
Copy link
Member

schalkms commented Feb 4, 2023

@mmorozkov No blockers left. I'll merge this. If @BraisGabin has anything to add, we will create a new PR.
Btw, sorry for the delay. That shouldn't have happened.

@schalkms schalkms merged commit d147306 into detekt:main Feb 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BraisGabin BraisGabin BraisGabin left review comments

@cortinico cortinico cortinico approved these changes

Assignees
No one assigned
Labels
rules
Projects
None yet
Milestone
1.23.0
Development

Successfully merging this pull request may close these issues.

ElseCaseInsteadOfExhaustiveWhen config to fail when there's only one unused type/value
5 participants
@mmorozkov @G00fY2 @BraisGabin @schalkms @cortinico