Group console violation reports by file #1852

sowmyav24 · 2019-08-28T04:18:01Z

This PR addresses #1355

Open questions:

Should we make type of report to be displayed configurable to the user ?
The output format would be similar to the existing report and all the violations of a file appear together. Do we need to make any changes to the output format ?

arturbosch

Thanks for contributing to this feature.
Extending the public API would introduce a breaking change, so I'm against adding a new function to the interface when we could implement this Report type by a custom ConsoleReport.
This way we could provide a selection of FindingsRepots and let the user choose which to use in the config file.

sowmyav24 · 2019-08-28T06:12:34Z

Thank you for the suggestion.
Will update the PR accordingly.

sowmyav24 · 2019-08-29T01:59:57Z

we could provide a selection of FindingsRepots and let the user choose which to use in the config file.

In the updated PR, it is a boolean value. Would it be better if it is changed it to Enum as it will allow the users to choose multiple console reports or would a boolean value suffice ?

schalkms

Thank you very much for picking up this issue! That's a nice 1st contribution! :)

In my opinion this PR is ready for review.
Please take a look at my comments.
The report FileBasedFindingsReport should be added to the default-detekt-config.yml file.
Perhaps a comment should be included, which states that only one of the reports can be activated at the same time (either FindingsReport or FileBasedFindingsReport)
https://github.com/arturbosch/detekt/blob/ed77f0c9ebff73a50348c76782e6dcb1bd7d7b08/detekt-cli/src/main/resources/default-detekt-config.yml#L21-L25

detekt-cli/src/main/kotlin/io/gitlab/arturbosch/detekt/cli/console/DebtSumming.kt

...t-cli/src/test/kotlin/io/gitlab/arturbosch/detekt/cli/console/FileBasedFindingsReportSpec.kt

detekt-cli/src/test/kotlin/io/gitlab/arturbosch/detekt/cli/TestFactory.kt

detekt-cli/src/main/kotlin/io/gitlab/arturbosch/detekt/cli/console/FileBasedFindingsReport.kt

detekt-cli/src/main/kotlin/io/gitlab/arturbosch/detekt/cli/console/FindingsReport.kt

detekt-cli/src/main/kotlin/io/gitlab/arturbosch/detekt/cli/console/FileBasedFindingsReport.kt

codecov-io · 2019-08-29T11:27:58Z

Codecov Report

❗ No coverage uploaded for pull request base (master@fb134ca). Click here to learn what that means.
The diff coverage is 89.13%.

@@            Coverage Diff            @@
##             master    #1852   +/-   ##
=========================================
  Coverage          ?   80.36%           
  Complexity        ?     1968           
=========================================
  Files             ?      328           
  Lines             ?     5551           
  Branches          ?     1032           
=========================================
  Hits              ?     4461           
  Misses            ?      557           
  Partials          ?      533

Impacted Files	Coverage Δ	Complexity Δ
...ab/arturbosch/detekt/api/FileBasedConsoleReport.kt	`100% <100%> (ø)`	`1 <1> (?)`
...ab/arturbosch/detekt/cli/console/FindingsReport.kt	`93.33% <100%> (ø)`	`6 <0> (?)`
...n/io/gitlab/arturbosch/detekt/cli/ReportLocator.kt	`74.07% <66.66%> (ø)`	`4 <0> (?)`
...rturbosch/detekt/cli/console/DebtSummingPrinter.kt	`83.33% <83.33%> (ø)`	`2 <2> (?)`
...osch/detekt/cli/console/FileBasedFindingsReport.kt	`95.23% <95.23%> (ø)`	`7 <7> (?)`

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 fb134ca...96b0f36. Read the comment docs.

sowmyav24 · 2019-08-29T15:25:11Z

The report FileBasedFindingsReport should be added to the default-detekt-config.yml file.
Perhaps a comment should be included, which states that only one of the reports can be activated at the same time (either FindingsReport or FileBasedFindingsReport)
https://github.com/arturbosch/detekt/blob/ed77f0c9ebff73a50348c76782e6dcb1bd7d7b08/detekt-cli/src/main/resources/default-detekt-config.yml#L21-L25

When the FileBasedFindingsReport is added in default-detekt-config.yml , the build fails.
In addition, added the FileBasedFindingsReport in io.gitlab.arturbosch.detekt.api.ConsoleReport as it is used by Service loader. Please let me know if I am mistaken here.

schalkms

Sorry, I should have expressed that clearer.
Additionally, you need to add the report class to a file in the META-INF directory.

detekt-cli/src/main/resources/META-INF/services/io.gitlab.arturbosch.detekt.api.ConsoleReport

PS: thanks for the incorporated changes from the review!

detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/FileBasedConsoleReport.kt

detekt-cli/src/main/kotlin/io/gitlab/arturbosch/detekt/cli/console/DebtSummingPrinter.kt

detekt-cli/src/main/kotlin/io/gitlab/arturbosch/detekt/cli/console/FileBasedFindingsReport.kt

...t-cli/src/test/kotlin/io/gitlab/arturbosch/detekt/cli/console/FileBasedFindingsReportSpec.kt

sowmyav24 · 2019-08-30T15:14:39Z

detekt-cli/src/main/resources/META-INF/services/io.gitlab.arturbosch.detekt.api.ConsoleReport

Yes , this has been done.
But i am unable to add FileBasedFindingsReport to default-detekt-config.yml. This gets reset when build is run and if added, the CI fails.

schalkms · 2019-08-30T15:16:05Z

CI fails because of other issues. (not related to this PR)

sowmyav24 · 2019-08-30T15:21:10Z

Ah okay, but the exclusion gets removed once the build runs in local. I have removed the exclusion from default-detekt-config.yml for the time being.

schalkms

Thanks for fixing the merge conflict. We are sorry that your contribution interfered with the bug fix.
Please look at my last review comment. Everything else looks fine.
Anyway, you don't need to force push. We squash and merge the contribution anyhow.
This makes it easier and less time consuming for the reviewer to see what changed between review round n-1 and n.

...t-cli/src/test/kotlin/io/gitlab/arturbosch/detekt/cli/console/FileBasedFindingsReportSpec.kt

detekt-cli/src/main/kotlin/io/gitlab/arturbosch/detekt/cli/console/FindingsReport.kt

detekt-cli/src/main/kotlin/io/gitlab/arturbosch/detekt/cli/console/FileBasedFindingsReport.kt

detekt-cli/src/main/kotlin/io/gitlab/arturbosch/detekt/cli/console/DebtSummingPrinter.kt

arturbosch

Sorry for the late response!
I'm not sure why we need an additional flag and a new base class for FileBasedFindingsReport?
I expect this new kind of report to be just a ConsoleReport which does the new ordering. It is excluded by default.
Moreover I see the need to refactor the reports part in the config to have includes and excludes because now if a report is not excluded it means it is included.
@schalkms @sowmyav24 please elaborate why we need a new flag for it.

arturbosch · 2019-09-11T07:46:55Z

detekt-cli/src/main/kotlin/io/gitlab/arturbosch/detekt/cli/console/DebtSummingPrinter.kt

+
+class DebtSummingPrinter {
+
+    fun printDebtInformation(


Seems like this could just be a function?

Sorry for the late response!
I'm not sure why we need an additional flag and a new base class for FileBasedFindingsReport?
I expect this new kind of report to be just a ConsoleReport which does the new ordering. It is excluded by default.
Moreover I see the need to refactor the reports part in the config to have includes and excludes because now if a report is not excluded it means it is included.
@schalkms @sowmyav24 please elaborate why we need a new flag for it.

If I am to understand correctly, if the additional flag is not there, there would be two console reports, one is the existing report, one is the re-ordered report and both the reports would contain the same information but in a different format.

Yes, to prevent this we should exclude this new report by default.
Users have to activate it and exclude the default one.

Okay got it. The report is excluded now and the additional flag is removed. Please let me know if any other documentation or changes are left for this.

The build passes in local, but fails for compilation in the CI

detekt-cli/src/main/resources/default-detekt-config.yml

detekt-cli/src/test/resources/grouped-findings-report.txt

…r for section as file names and not rulesets

detekt-cli/src/test/kotlin/io/gitlab/arturbosch/detekt/cli/TestFactory.kt

detekt-cli/src/main/kotlin/io/gitlab/arturbosch/detekt/cli/console/DebtSummingPrinter.kt

detekt-cli/src/test/resources/grouped-findings-report.txt

...r/src/main/kotlin/io/gitlab/arturbosch/detekt/generator/printer/rulesetpage/ConfigPrinter.kt

…ebtSumming

* Group console violation reports by file * Move empty check on findings before calling the debt printer * Remove flag for file based findings report and exclude it by default * Change format of console report for grouping by files. Have the header for section as file names and not rulesets * Exclude FileBasedFindingsReport and move methods to print debt into debtSumming * Rename test parameter to ruleName from ruleSet

sowmyav24 mentioned this pull request Aug 28, 2019

Grouped output #1355

Closed

arturbosch reviewed Aug 28, 2019

View reviewed changes

sowmyav24 force-pushed the file-grouped-console-output branch 2 times, most recently from c84372e to 507e851 Compare August 29, 2019 01:57

sowmyav24 force-pushed the file-grouped-console-output branch from 507e851 to 88d858d Compare August 29, 2019 02:35

schalkms reviewed Aug 29, 2019

View reviewed changes

sowmyav24 force-pushed the file-grouped-console-output branch from 88d858d to a6a8f1e Compare August 29, 2019 11:27

sowmyav24 force-pushed the file-grouped-console-output branch from a6a8f1e to 072d3c6 Compare August 29, 2019 12:55

sowmyav24 marked this pull request as ready for review August 29, 2019 13:27

sowmyav24 force-pushed the file-grouped-console-output branch from 072d3c6 to 8f41a3f Compare August 29, 2019 15:21

schalkms reviewed Aug 30, 2019

View reviewed changes

sowmyav24 force-pushed the file-grouped-console-output branch from 8f41a3f to e865efc Compare August 30, 2019 14:52

sowmyav24 force-pushed the file-grouped-console-output branch from e865efc to dab11bf Compare August 30, 2019 15:15

sowmyav24 force-pushed the file-grouped-console-output branch 3 times, most recently from 31a0098 to 69a23cd Compare August 31, 2019 13:50

Group console violation reports by file

b8b4cc8

sowmyav24 force-pushed the file-grouped-console-output branch from 69a23cd to b8b4cc8 Compare August 31, 2019 13:55

schalkms approved these changes Aug 31, 2019

View reviewed changes

Move empty check on findings before calling the debt printer

1dab1f4

schalkms requested a review from arturbosch September 1, 2019 09:10

arturbosch reviewed Sep 11, 2019

View reviewed changes

Remove flag for file based findings report and exclude it by default

dc63c0c

sowmyav24 force-pushed the file-grouped-console-output branch from 546591d to dc63c0c Compare September 13, 2019 02:55

arturbosch reviewed Sep 13, 2019

View reviewed changes

detekt-cli/src/main/resources/default-detekt-config.yml Outdated Show resolved Hide resolved

arturbosch reviewed Sep 13, 2019

View reviewed changes

detekt-cli/src/test/resources/grouped-findings-report.txt Outdated Show resolved Hide resolved

arturbosch approved these changes Sep 13, 2019

View reviewed changes

arturbosch added this to the 1.1.0 milestone Sep 13, 2019

Change format of console report for grouping by files. Have the heade…

e25a9bc

…r for section as file names and not rulesets

arturbosch reviewed Sep 16, 2019

View reviewed changes

detekt-cli/src/test/kotlin/io/gitlab/arturbosch/detekt/cli/TestFactory.kt Outdated Show resolved Hide resolved

arturbosch reviewed Sep 16, 2019

View reviewed changes

detekt-cli/src/main/kotlin/io/gitlab/arturbosch/detekt/cli/console/DebtSummingPrinter.kt Outdated Show resolved Hide resolved

arturbosch reviewed Sep 16, 2019

View reviewed changes

detekt-cli/src/test/resources/grouped-findings-report.txt Show resolved Hide resolved

arturbosch reviewed Sep 16, 2019

View reviewed changes

...r/src/main/kotlin/io/gitlab/arturbosch/detekt/generator/printer/rulesetpage/ConfigPrinter.kt Outdated Show resolved Hide resolved

Exclude FileBasedFindingsReport and move methods to print debt into d…

4ddbdf7

…ebtSumming

Rename test parameter to ruleName from ruleSet

96b0f36

sowmyav24 force-pushed the file-grouped-console-output branch from 21903b1 to 96b0f36 Compare September 17, 2019 01:52

arturbosch merged commit f7bbc90 into detekt:master Sep 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Group console violation reports by file #1852

Group console violation reports by file #1852

sowmyav24 commented Aug 28, 2019

arturbosch left a comment

sowmyav24 commented Aug 28, 2019

sowmyav24 commented Aug 29, 2019

schalkms left a comment •

edited

Loading

codecov-io commented Aug 29, 2019 •

edited

Loading

sowmyav24 commented Aug 29, 2019

schalkms left a comment •

edited

Loading

sowmyav24 commented Aug 30, 2019

schalkms commented Aug 30, 2019

sowmyav24 commented Aug 30, 2019

schalkms left a comment

arturbosch left a comment

arturbosch Sep 11, 2019

sowmyav24 Sep 11, 2019

arturbosch Sep 12, 2019

sowmyav24 Sep 13, 2019

sowmyav24 Sep 13, 2019

Group console violation reports by file #1852

Group console violation reports by file #1852

Conversation

sowmyav24 commented Aug 28, 2019

arturbosch left a comment

Choose a reason for hiding this comment

sowmyav24 commented Aug 28, 2019

sowmyav24 commented Aug 29, 2019

schalkms left a comment • edited Loading

Choose a reason for hiding this comment

codecov-io commented Aug 29, 2019 • edited Loading

Codecov Report

sowmyav24 commented Aug 29, 2019

schalkms left a comment • edited Loading

Choose a reason for hiding this comment

sowmyav24 commented Aug 30, 2019

schalkms commented Aug 30, 2019

sowmyav24 commented Aug 30, 2019

schalkms left a comment

Choose a reason for hiding this comment

arturbosch left a comment

Choose a reason for hiding this comment

arturbosch Sep 11, 2019

Choose a reason for hiding this comment

sowmyav24 Sep 11, 2019

Choose a reason for hiding this comment

arturbosch Sep 12, 2019

Choose a reason for hiding this comment

sowmyav24 Sep 13, 2019

Choose a reason for hiding this comment

sowmyav24 Sep 13, 2019

Choose a reason for hiding this comment

schalkms left a comment •

edited

Loading

codecov-io commented Aug 29, 2019 •

edited

Loading

schalkms left a comment •

edited

Loading