-
-
Notifications
You must be signed in to change notification settings - Fork 794
Report SwallowedException on catchParameter #4158
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
Conversation
This improves the ergonomics so that users can minimize the suppression scope.
Codecov Report
@@ Coverage Diff @@
## main #4158 +/- ##
============================================
- Coverage 83.44% 83.43% -0.02%
- Complexity 3177 3190 +13
============================================
Files 465 465
Lines 9104 9121 +17
Branches 1775 1780 +5
============================================
+ Hits 7597 7610 +13
Misses 572 572
- Partials 935 939 +4
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am using the detekt repo itself as the integration test for the PR change, otherwise I probably need to write an integration test in detekt-core which does not seem like the right place because the change is in
detekt-rules-exceptions
.
No you don't need to write any integration test. The engine that runs the rules in test honor the @Suppress
so you can unit test it easily.
Or, if you prefer, you can test that the reported text is the correct one.
...ns/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/exceptions/TooGenericExceptionCaught.kt
Outdated
Show resolved
Hide resolved
try { | ||
@Suppress("SpreadOperator") | ||
jCommander.parse(*args) | ||
} catch (ex: ParameterException) { | ||
} catch (@Suppress("SwallowedException") ex: ParameterException) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will it still accept suppressing on try
? because a change shouldn't force everyone to move suppressions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, try is a parent of catch so the suppression engine should allow now both.
Now this fixes #4153 too. |
@BraisGabin in #4153 you said add a test, I don't see it here. |
This improves the ergonomics so that users can minimize the suppression scope.
Addresses #4154
I am using the detekt repo itself as the integration test for the PR change, otherwise I probably need to write an integration test in detekt-core which does not seem like the right place because the change is in
detekt-rules-exceptions
.