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

Correct documentation/recommendation of EmptyCatchBlock rule #2603

Merged
merged 3 commits into from Apr 14, 2020
Merged

Correct documentation/recommendation of EmptyCatchBlock rule #2603

merged 3 commits into from Apr 14, 2020

Conversation

Whathecode
Copy link
Contributor

@Whathecode Whathecode commented Apr 14, 2020

Fixes #2602

The current code smell recommendation for empty catch blocks is wrong:

Empty blocks of code serve no purpose and should be removed.

Empty catch blocks cannot be removed, since that results in compilation errors.

This PR updates the documentation to give more guidance in line with how the rule is currently implemented. The recommendation seems to be to clarify the exception is intentionally ignored by naming the exception as such.

However, this also raises the question whether this should really be part of the 'empty block' ruleset. To me this is more akin to the TooGenericExceptionCaught rule. This is exemplified by the fact that I had to modify the EmptyRule class in order to be able to modify the messages. In addition, one could argue this requires a higher severity than the 'minor' severity inherited from EmptyRule.

@Whathecode Whathecode changed the title Correct documentation/recommendation of EmptyCatchBlock rule. Correct documentation/recommendation of EmptyCatchBlock rule Apr 14, 2020
@codecov-io
Copy link

codecov-io commented Apr 14, 2020

Codecov Report

Merging #2603 into master will increase coverage by 1.74%.
The diff coverage is 90.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2603      +/-   ##
============================================
+ Coverage     25.17%   26.92%   +1.74%     
+ Complexity      395      394       -1     
============================================
  Files           379      379              
  Lines          7420     6734     -686     
  Branches       1225     1224       -1     
============================================
- Hits           1868     1813      -55     
+ Misses         5423     4792     -631     
  Partials        129      129              
Impacted Files Coverage Δ Complexity Δ
.../gitlab/arturbosch/detekt/rules/empty/EmptyRule.kt 47.05% <83.33%> (+17.64%) 3.00 <2.00> (+1.00)
...b/arturbosch/detekt/rules/empty/EmptyCatchBlock.kt 55.55% <100.00%> (+26.98%) 1.00 <1.00> (ø)
...itlab/arturbosch/detekt/api/internal/Validation.kt 33.33% <0.00%> (-16.67%) 0.00% <0.00%> (ø%)
.../gitlab/arturbosch/detekt/cli/console/Colorizer.kt 61.53% <0.00%> (-9.90%) 0.00% <0.00%> (ø%)
...h/detekt/api/internal/DisabledAutoCorrectConfig.kt 55.55% <0.00%> (-8.09%) 4.00% <0.00%> (ø%)
...otlin/io/gitlab/arturbosch/detekt/core/TaskPool.kt 33.33% <0.00%> (-6.67%) 3.00% <0.00%> (ø%)
...in/io/gitlab/arturbosch/detekt/api/SingleAssign.kt 60.00% <0.00%> (-6.67%) 3.00% <0.00%> (ø%)
...in/kotlin/io/gitlab/arturbosch/detekt/api/Issue.kt 50.00% <0.00%> (-5.56%) 2.00% <0.00%> (ø%)
...n/io/gitlab/arturbosch/detekt/api/ConsoleReport.kt 75.00% <0.00%> (-5.00%) 4.00% <0.00%> (ø%)
...tlab/arturbosch/detekt/api/internal/EmptyConfig.kt 66.66% <0.00%> (-4.77%) 4.00% <0.00%> (ø%)
... and 261 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca6591e...9dbf804. Read the comment docs.

Copy link
Member

@schalkms schalkms 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 improving the documentation! This was really a necessary step and helps to guide the user.
Please see my attached comments.

Whathecode and others added 2 commits April 14, 2020 12:33
Co-Authored-By: M Schalk <30376729+schalkms@users.noreply.github.com>
@schalkms schalkms merged commit eb83202 into detekt:master Apr 14, 2020
@arturbosch arturbosch added this to the 1.8.0 milestone Apr 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect (or unclear) EmptyCatchBlock rule
4 participants