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

Use notifications instead of println #1818

Merged
merged 7 commits into from
Aug 17, 2019
Merged

Use notifications instead of println #1818

merged 7 commits into from
Aug 17, 2019

Conversation

arturbosch
Copy link
Member

@arturbosch arturbosch commented Aug 14, 2019

Closes - #1797

a config like:

processors:
  active: true
  exclude:
    - 'DetektProgressListener'
    - 'FunctionCountProcessor'
    - 'PropertyCountProcessor'
    - 'ClassCountProcessor'
    - 'PackageCountProcessor'
    - 'KtFileCountProcessor'

console-reports:
  active: true
  exclude:
    - 'ProjectStatisticsReport'
    - 'ComplexityReport'
    - 'NotificationReport'
    - 'FindingsReport'
    - 'BuildFailureReport'

won't print anything.

I will write a guide on how to turn detekt silent for your CI in a separate PR.

@arturbosch arturbosch added this to the 1.0.1 milestone Aug 14, 2019
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.

Top!

@schalkms
Copy link
Member

schalkms commented Aug 15, 2019

Codecov is back once again.
I'd be interested on the circumstances. When does this integration work? To me it seems to be rather random. 🔢
I investigated this in #1810 and #1811. From the CI log it seems to me that the report gets generated successfully but it's not uploaded correctly to codecov.io, even though it mentions a successful upload.

@codecov-io
Copy link

codecov-io commented Aug 15, 2019

Codecov Report

Merging #1818 into master will decrease coverage by 0.09%.
The diff coverage is 68.96%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master    #1818     +/-   ##
===========================================
- Coverage     79.75%   79.66%   -0.1%     
- Complexity     1932     1934      +2     
===========================================
  Files           323      324      +1     
  Lines          5497     5502      +5     
  Branches       1019     1020      +1     
===========================================
- Hits           4384     4383      -1     
- Misses          589      594      +5     
- Partials        524      525      +1
Impacted Files Coverage Δ Complexity Δ
.../io/gitlab/arturbosch/detekt/cli/runners/Runner.kt 0% <0%> (ø) 0 <0> (ø) ⬇️
...turbosch/detekt/api/internal/SimpleNotification.kt 100% <100%> (ø) 1 <1> (?)
...ab/arturbosch/detekt/cli/DetektProgressListener.kt 75% <100%> (ø) 3 <1> (ø) ⬇️
...rturbosch/detekt/cli/console/NotificationReport.kt 100% <100%> (ø) 3 <2> (ø) ⬇️
...ab/arturbosch/detekt/cli/console/FindingsReport.kt 91.3% <100%> (-8.7%) 6 <0> (+1)
...in/io/gitlab/arturbosch/detekt/cli/OutputFacade.kt 80.76% <100%> (ø) 10 <0> (ø) ⬇️
...ekt/generator/printer/rulesetpage/ConfigPrinter.kt 72.97% <50%> (-4.17%) 7 <3> (ø)
...itlab/arturbosch/detekt/core/ProcessingSettings.kt 76.74% <0%> (-2.33%) 15% <0%> (-1%)
...n/io/gitlab/arturbosch/detekt/core/DetektResult.kt 90% <0%> (+10%) 6% <0%> (+1%) ⬆️

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 3c22b1b...01960cb. Read the comment docs.

@arturbosch
Copy link
Member Author

arturbosch commented Aug 16, 2019

I've extracted the BuildFailure logic to a common file, the BuildFailureReport now uses it and in a much cleaner way and the runner is now responsible for throwing that exception.

Please review that last commit.

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.

I had a very hard time reviewing this last commit, because it contains so many changes.
Basically, detekt now throws exceptions of type BuildFailure instead of printing to the system.out.


runCatching { Runner(cliArgs).execute() }
.onSuccess { assertThat(Files.readAllLines(tmpReport)).hasSize(1) }
.onFailure { fail("should not fail with a BuildFailure exception") }
Copy link
Member

Choose a reason for hiding this comment

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

Weird test construct

Copy link
Member

Choose a reason for hiding this comment

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

Does it test a successful or a failed run?

Copy link
Member Author

Choose a reason for hiding this comment

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

Your right, the catch and failure block is not necessary for this test.

@arturbosch
Copy link
Member Author

arturbosch commented Aug 17, 2019

Thanks for the feedback, I will split the changes into two PRs.
Edit: Or maybe not, will comment your comments :)

@arturbosch arturbosch merged commit 3666126 into master Aug 17, 2019
@arturbosch arturbosch deleted the do-not-use-println branch August 17, 2019 18:09
smyachenkov pushed a commit to smyachenkov/detekt that referenced this pull request Dec 9, 2019
* Make use of notifications instead of printing directly to the console

* Use spaces in config printer

* Do not print rule set ids when no issues are present for that rule set

* Extract build failure logic to a common file, just throw in runner

* Simplify testcase

* Rearrange test cases to make clear that rule set ids are not printed when no findings are found

* Make clear that we should eagerly test the existence of a rule before trying to run it and fail on not found
smyachenkov pushed a commit to smyachenkov/detekt that referenced this pull request Dec 9, 2019
* Make use of notifications instead of printing directly to the console

* Use spaces in config printer

* Do not print rule set ids when no issues are present for that rule set

* Extract build failure logic to a common file, just throw in runner

* Simplify testcase

* Rearrange test cases to make clear that rule set ids are not printed when no findings are found

* Make clear that we should eagerly test the existence of a rule before trying to run it and fail on not found
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