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

Add default values to SwallowedException rule #2661

Conversation

juliabirkett
Copy link
Contributor

@juliabirkett juliabirkett commented May 8, 2020

There are some rules that have default values in the documentation but those values are not implemented in the code.
This is a bug mapped here #2597

This PR adds default values to SwallowedException rule as mapped in the documentation.


⚠️ This is my very first contribution to OS so sorry if I'm doing anything wrong. If this PR is OK, I'm gonna add more default values to other rules.

@juliabirkett juliabirkett changed the title Add defaults to swallowed exception rule Add default values to SwallowedException rule May 8, 2020
Copy link
Member

@BraisGabin BraisGabin left a comment

Choose a reason for hiding this comment

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

First of all, thank you to take the time to fix this issue! I added some comment below

Copy link
Member

@BraisGabin BraisGabin left a comment

Choose a reason for hiding this comment

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

LGTM! Great first contribution :)

The error that you are getting in CI is kind of odd. I think that what happen is that you need to add the imports for java.net.MalformedURLException and java.text.ParseException because they are not imported by default. You can test it in your computer with:
./gradlew :detekt-rules:test -Pcompile-test-snippets=true

By default the code in the snippets is not compiled, it's only compiled in the ubuntu/java8.

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 the nice 1st contribution! No worries. Everyone starts out as a 1st time contributor someday. I have a few comments regarding your PR.
If you have questions, please ping me directly.

It's nice to hear that you want to help out further with subsequent PRs. That's cool! 🙂

@arturbosch arturbosch added this to the 1.10.0 milestone May 25, 2020
@arturbosch arturbosch force-pushed the add-defaults-to-swallowed-exception-rule branch from c8890fa to 582b178 Compare June 1, 2020 09:31
@arturbosch
Copy link
Member

@juliabirkett @schalkms I've added a commit to get this into the next release soon :)

@detekt detekt deleted a comment from codecov bot Jun 1, 2020
@codecov
Copy link

codecov bot commented Jun 1, 2020

Codecov Report

Merging #2661 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2661      +/-   ##
============================================
+ Coverage     80.44%   80.45%   +0.01%     
  Complexity     2303     2303              
============================================
  Files           378      378              
  Lines          6949     6954       +5     
  Branches       1262     1262              
============================================
+ Hits           5590     5595       +5     
  Misses          729      729              
  Partials        630      630              
Impacted Files Coverage Δ Complexity Δ
...osch/detekt/rules/exceptions/SwallowedException.kt 74.50% <100.00%> (+2.77%) 17.00 <0.00> (ø)

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 e1f213b...dddfa17. Read the comment docs.

@arturbosch arturbosch force-pushed the add-defaults-to-swallowed-exception-rule branch from 582b178 to dddfa17 Compare June 1, 2020 09:47
@arturbosch arturbosch merged commit a763749 into detekt:master Jun 1, 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.

4 participants