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

Modify default behavior to not output unless errors are found. Adding a verbose flag which will have legacy behavior #2258

Closed
wants to merge 6 commits into from

Conversation

zsmoore
Copy link
Contributor

@zsmoore zsmoore commented Jan 15, 2020

Adding clli argument and gradle plugin functionality to enable verbose logging.

On our project we only want to log to console if an error occurs for cleaner logging,

Swapped default logic to not output unless errors present or verbose flag enabled

@codecov-io
Copy link

codecov-io commented Jan 15, 2020

Codecov Report

Merging #2258 into master will decrease coverage by <.01%.
The diff coverage is 89.47%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2258      +/-   ##
============================================
- Coverage     81.45%   81.44%   -0.01%     
- Complexity     2114     2117       +3     
============================================
  Files           349      349              
  Lines          6054     6063       +9     
  Branches       1106     1109       +3     
============================================
+ Hits           4931     4938       +7     
  Misses          532      532              
- Partials        591      593       +2
Impacted Files Coverage Δ Complexity Δ
.../arturbosch/detekt/cli/runners/SingleRuleRunner.kt 83.33% <0%> (-2.78%) 6 <0> (ø)
.../kotlin/io/gitlab/arturbosch/detekt/cli/CliArgs.kt 100% <100%> (ø) 6 <0> (ø) ⬇️
...itlab/arturbosch/detekt/core/ProcessingSettings.kt 90.32% <100%> (+0.49%) 20 <2> (+1) ⬆️
.../io/gitlab/arturbosch/detekt/cli/runners/Runner.kt 87.75% <100%> (+0.52%) 10 <0> (ø) ⬇️
...in/io/gitlab/arturbosch/detekt/cli/OutputFacade.kt 81.48% <100%> (+0.71%) 12 <3> (+2) ⬆️
...n/io/gitlab/arturbosch/detekt/core/DetektFacade.kt 48.83% <75%> (+0.05%) 3 <0> (ø) ⬇️

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 a517db9...39afd27. 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.

I have some questions. Please see my comments for that.
Anyway, I'm fine with changing the API here.
I'll add a second maintainer to the review list.

@arturbosch
Copy link
Member

Cool, I had something similiar in mind, calling it --silent.
I will take another look at this PR later.
With the current version you can try https://arturbosch.github.io/detekt/howto-silent-reports.html for now.

@BraisGabin
Copy link
Member

I love this PR! I think that no output should be the default. And add -v if you want the verbose mode.

As a bonus we can have the quiet mode -q. It should print nothing at all. It just create the reports.

@schalkms
Copy link
Member

schalkms commented Jan 16, 2020

I think that no output should be the default. And add -v if you want the verbose mode.

That's a good idea! I agree with that. Then we should call the flag introduced with this PR --verbose = false
I hope that also fits your needs @zsmoore

@zsmoore zsmoore changed the title Add cli argument to suppress console logs when no errors found. Modify default behavior to not output unless errors are found. Adding a verbose flag which will have legacy behavior Jan 17, 2020
@zsmoore
Copy link
Contributor Author

zsmoore commented Jan 17, 2020

Updated PR to default to no output unless error detekt should fail build or new flag verbose is enabled

@@ -53,6 +53,9 @@ class OutputFacade(
}

private fun handleConsoleReport(report: ConsoleReport, result: Detektion) {
report.print(settings.outPrinter, result)
if (settings.verbose ||
config.maxIssues().isValidAndSmallerOrEqual(result.getOrComputeWeightedAmountOfIssues(config))) {
Copy link
Member

Choose a reason for hiding this comment

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

We only want to print something if the max issue count is exceeded, but here we just print something if the actual issue count is below the max, right?

@schalkms
Copy link
Member

Cool! Thanks for making these changes and bringing real value to our users.

@BraisGabin
Copy link
Member

BraisGabin commented Jan 17, 2020

I would add -v as an alias of --verbose (it's kind of a standard).

If I don't use the --verbose flag in a project without issues I get no output (expected behaviour 👍). But, if I do the same with a project with issues I get the error report (expected behaviour 👍) but I'm getting this too:

detekt finished in 17751 ms.
Successfully generated HTML report at report.html
Build failed with 3178 weighted issues (threshold defined was 10).
io.gitlab.arturbosch.detekt.cli.BuildFailure: Build failed with 3178 weighted issues (threshold defined was 10).

Probably we should show the summary "Build failed with 3178 weighted issues (threshold defined was 10)." But the time and the generated reports should not be there. What do you think?

@schalkms
Copy link
Member

@BraisGabin I think we should keep it as simple as possible now.

@BraisGabin
Copy link
Member

At least the -v should be added, I think. The other points can be fixed later.

@schalkms
Copy link
Member

At least the -v should be added

Agreed.

@arturbosch
Copy link
Member

arturbosch commented Jan 19, 2020

I'm not really sure we need a --verbose flag.
Introducing if-else and new parameters for the DetektFacade introduces complexity.

We could achieve the same silent output when changing the Extension's defaults.
I will take a further look at our extension mechanism. As we are now changing the maxIssues default, lets change the printing output too.
Please take a look at #2268.

@arturbosch arturbosch added this to the 1.5.0 milestone Jan 19, 2020
@arturbosch
Copy link
Member

@zsmoore thanks for bringing up this suggestions. We have decided to merge in the simpler solutions and opened #2267 for a major update. A more silent detekt with fail fast default will be released in 1.5.0 :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants