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

Throw error instead of logging as error in analysis phase #3193

Merged
merged 3 commits into from
Nov 5, 2020

Conversation

chao2zhang
Copy link
Member

@chao2zhang chao2zhang commented Nov 2, 2020

This PR addresses #3183

A few changes worth calling out:

  • Move a few public elements from Junk.kt to private elements Analyzer.kt
  • Add a new primary constructor for UnexpectedError which accepts the message
  • Rename a few places where the name Detektor appears to Analyzer. Introduce tooling api module #2861 renamed the class from Detektor to Analyzer but did not update all references like variable names.

With this PR, users will see the following output in the console:

~/DetektSilentError(main*) » ./gradlew detekt                                                                                                                                     1 ↵ cazhang@cazhang-mn4
> Task :detekt FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':detekt'.
> io.github.detekt.tooling.api.AnalysisError: Analyzing /Users/cazhang/DetektSilentError/src/main/kotlin/A.kt led to an exception. 
  The original exception message was: java.lang.ClassCastException: java.lang.String cannot be cast to java.lang.Number
  Running detekt '1.14.2' on Java '1.8.0_212-b10' on OS 'Mac OS X'
  If the exception message does not help, please feel free to create an issue on our GitHub page.

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. Run with --scan to get full insights.

* Get more help at https://help.gradle.org

BUILD FAILED in 749ms
1 actionable task: 1 executed
---------------------------------

There is one more usage of LoggingAware.error(), although tangential to this PR, it would be great to remove LoggingAware.error() because logging as an error is not treating the severity correctly. When an exception happens, we should surface the error back to the user rather than logging and telling the user that the command completed successfully.

@codecov
Copy link

codecov bot commented Nov 2, 2020

Codecov Report

Merging #3193 into master will increase coverage by 0.21%.
The diff coverage is 91.66%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3193      +/-   ##
============================================
+ Coverage     79.84%   80.05%   +0.21%     
- Complexity     2610     2613       +3     
============================================
  Files           439      439              
  Lines          7953     7953              
  Branches       1516     1516              
============================================
+ Hits           6350     6367      +17     
+ Misses          790      774      -16     
+ Partials        813      812       -1     
Impacted Files Coverage Δ Complexity Δ
...in/kotlin/io/gitlab/arturbosch/detekt/core/Junk.kt 100.00% <ø> (+40.00%) 0.00 <0.00> (ø)
...otlin/io/gitlab/arturbosch/detekt/core/Analyzer.kt 81.08% <90.00%> (+13.43%) 9.00 <0.00> (+3.00)
...gitlab/arturbosch/detekt/core/tooling/Lifecycle.kt 100.00% <100.00%> (ø) 0.00 <0.00> (ø)
.../gitlab/arturbosch/detekt/api/internal/Versions.kt 87.50% <0.00%> (+25.00%) 0.00% <0.00%> (ø%)
...otlin/io/gitlab/arturbosch/detekt/core/TaskPool.kt 100.00% <0.00%> (+33.33%) 6.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 a738300...270f74b. 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 this PR. This one looks good. I think the mentioned improvements should be tackled in a subsequent PR. What do you think? May I ask you to please create an issue containing this improvement for tracking purposes?

@arturbosch arturbosch added this to the 1.15.0 milestone Nov 2, 2020
@chao2zhang
Copy link
Member Author

Thanks for this PR. This one looks good. I think the mentioned improvements should be tackled in a subsequent PR. What do you think? May I ask you to please create an issue containing this improvement for tracking purposes?

See #3183. This PR is already linked to the issue aforementioned.

@arturbosch arturbosch merged commit edaa3e9 into detekt:master Nov 5, 2020
sowmyav24 pushed a commit to sowmyav24/detekt that referenced this pull request Nov 8, 2020
* Throw error instead of logging as error in analysis phase

* Refactor to use IllegalStateException

* Revert DetektError change
arturbosch pushed a commit that referenced this pull request Nov 15, 2020
* Throw error instead of logging as error in analysis phase

* Refactor to use IllegalStateException

* Revert DetektError change
@chao2zhang chao2zhang deleted the master branch December 8, 2020 04:22
arturbosch pushed a commit that referenced this pull request Dec 21, 2020
* Throw error instead of logging as error in analysis phase

* Refactor to use IllegalStateException

* Revert DetektError change
arturbosch pushed a commit that referenced this pull request Jan 16, 2021
* Throw error instead of logging as error in analysis phase

* Refactor to use IllegalStateException

* Revert DetektError change
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.

None yet

3 participants