Adds issue details to findings on FindingsReport and FileBasedFindingsReporter #4464

gfreivasc · 2022-01-07T03:38:48Z

Addresses #4463

Changes FindingsReport to print with the current structure, but with messageOrDescription() + location.compact() instead of entity.compact() as the message gives more details and usually also mentions the entity. It tries to combine that layout with the degree of information found on LiteFindingsReport.

Message changes also apply to FileBasedFindingsReporter.

Example

fun main() {
   println( "Hello World!")
    if(1> 2)
        println("None")
}

`LiteFindingsReport`

/Users/gabrielfv/Projects/detekt/detekt-core/src/main/kotlin/io/gitlab/arturbosch/detekt/core/reporting/Reporting.kt:24:7: Missing spacing after "if" [SpacingAroundKeyword]
/Users/gabrielfv/Projects/detekt/detekt-core/src/main/kotlin/io/gitlab/arturbosch/detekt/core/reporting/Reporting.kt:23:1: Unexpected indentation (3) (should be 4) [Indentation]
/Users/gabrielfv/Projects/detekt/detekt-core/src/main/kotlin/io/gitlab/arturbosch/detekt/core/reporting/Reporting.kt:23:12: Unexpected spacing after "(" [SpacingAroundParens]
/Users/gabrielfv/Projects/detekt/detekt-core/src/main/kotlin/io/gitlab/arturbosch/detekt/core/reporting/Reporting.kt:25:9: Missing { ... } [MultiLineIfElse]

Current `FindingsReport`

formatting - 20min debt
	SpacingAroundKeyword - [] at /Users/gabrielfv/Projects/detekt/detekt-core/src/main/kotlin/io/gitlab/arturbosch/detekt/core/reporting/Reporting.kt:24:7
	Indentation - [] at /Users/gabrielfv/Projects/detekt/detekt-core/src/main/kotlin/io/gitlab/arturbosch/detekt/core/reporting/Reporting.kt:23:1
	SpacingAroundParens - [] at /Users/gabrielfv/Projects/detekt/detekt-core/src/main/kotlin/io/gitlab/arturbosch/detekt/core/reporting/Reporting.kt:23:12
	MultiLineIfElse - [] at /Users/gabrielfv/Projects/detekt/detekt-core/src/main/kotlin/io/gitlab/arturbosch/detekt/core/reporting/Reporting.kt:25:9

Overall debt: 20min

Modified `FindingsReport`

formatting - 25min debt
        MultiLineIfElse - [Missing { ... }] at /Users/gabrielfv/Projects/detekt/detekt-generator/src/main/kotlin/io/gitlab/arturbosch/detekt/generator/printer/defaultconfig/ConfigPrinter.kt:18:10
        SpacingAroundKeyword - [Missing spacing after "if"] at /Users/gabrielfv/Projects/detekt/detekt-generator/src/main/kotlin/io/gitlab/arturbosch/detekt/generator/printer/defaultconfig/ConfigPrinter.kt:17:8
        SpacingAroundParens - [Unexpected spacing after "("] at /Users/gabrielfv/Projects/detekt/detekt-generator/src/main/kotlin/io/gitlab/arturbosch/detekt/generator/printer/defaultconfig/ConfigPrinter.kt:16:13
        Indentation - [Unexpected indentation (5) (should be 4)] at /Users/gabrielfv/Projects/detekt/detekt-generator/src/main/kotlin/io/gitlab/arturbosch/detekt/generator/printer/defaultconfig/ConfigPrinter.kt:17:1
        Indentation - [Unexpected indentation (9) (should be 8)] at /Users/gabrielfv/Projects/detekt/detekt-generator/src/main/kotlin/io/gitlab/arturbosch/detekt/generator/printer/defaultconfig/ConfigPrinter.kt:18:1

Overall debt: 25min

Observations

These changes make it so that Entity::compact() method is no longer called internally. I chose to keep it as it's published API.

cortinico · 2022-01-07T11:45:13Z

Thanks for the PR @gfreivasc 👍
I'm just a bit worried that we will have DetailedFindingsReport, LiteFindingsReport and FindingsReport so it would be harder for users to understand what are the differences.

Shouldn't we just extend FindingsReport then?

codecov · 2022-01-07T11:47:47Z

Codecov Report

Merging #4464 (b6bee2b) into main (e1dc93e) will increase coverage by 0.00%.
The diff coverage is 90.90%.

@@            Coverage Diff            @@
##               main    #4464   +/-   ##
=========================================
  Coverage     84.12%   84.12%           
  Complexity     3310     3310           
=========================================
  Files           477      477           
  Lines         10916    10926   +10     
  Branches       2027     2029    +2     
=========================================
+ Hits           9183     9192    +9     
  Misses          700      700           
- Partials       1033     1034    +1

Impacted Files	Coverage Δ
...tlab/arturbosch/detekt/core/reporting/Reporting.kt	`93.87% <90.90%> (-1.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 e1dc93e...b6bee2b. Read the comment docs.

gfreivasc · 2022-01-07T13:20:04Z

While I personally don't think that's too much trouble, I see that LiteFindingsReporter was also a recent addition and this could be a little too much too soon. The reason I didn't initially consider changing FindingsReporter itself is because according to #4446 it's currently working as intended.

If you think we're good to make those changes to FindingsReporter, I could change my PR no problem.

Edit: Just in case we would move forward changing FindingsReporter, would we also want to change the format of FileBasedFindingsReport?

BraisGabin · 2022-01-08T16:36:32Z

I agree with @cortinico. I also vote to change FindingsReporter.

Edit: Just in case we would move forward changing FindingsReporter, would we also want to change the format of FileBasedFindingsReport?

Yes, I think that it would be good to keep both aligned.

gfreivasc · 2022-01-09T21:26:23Z

Changed the PR so now it changes FindingsReport and FileBasedFindingsReporter message format. I'll be looking for documentation references that would also require update.

BraisGabin

I never understand why compact is part of the Finding api... I think that we should deprecate it. But that's another topic.

cortinico · 2022-01-11T13:40:16Z

@gfreivasc Can you update the PR title to reflect the current diff?

gfreivasc · 2022-01-11T13:46:34Z

@cortinico changed the message

gfreivasc · 2022-01-11T19:57:27Z

@cortinico Had to fix tests from modules I had not run the tests for. They're all passing now.

cortinico · 2022-01-12T09:47:35Z

docs/pages/reporting.md

-NoUnusedImports - [] at /user/home/detekt/detekt-gradle-plugin/src/main/kotlin/io/gitlab/arturbosch/detekt/DetektPlugin.kt:10:1 - Signature=io.gitlab.arturbosch.detekt.DetektPlugin.kt:10
-NoConsecutiveBlankLines - [] at /user/home/detekt/detekt-gradle-plugin/src/main/kotlin/io/gitlab/arturbosch/detekt/DetektPlugin.kt:86:1 - Signature=io.gitlab.arturbosch.detekt.DetektPlugin.kt:86
-UnusedPrivateMember - [registerDetektJvmTasks] at /user/home/detekt/detekt-gradle-plugin/src/main/kotlin/io/gitlab/arturbosch/detekt/DetektPlugin.kt:17:5 - Signature=DetektPlugin.kt$DetektPlugin$private fun Project.registerDetektJvmTasks(extension: DetektExtension)
+EmptyFunctionBlock - [This empty block of code can be removed.] at /user/home/detekt/detekt-gradle-plugin/src/main/kotlin/io/gitlab/arturbosch/detekt/DetektPlugin.kt:14:42 - Signature=DetektPlugin.kt$DetektPlugin${ }


One last question: are there scenario where this text can either:

Be too long

Contain \n?

Askign as if so, we should escape it as it would screw up the final reporting.

As far as I'm aware of, there's no checks or anything to prevent these to be the case. These scenarios could also affect LiteFindingsReport. We could run a replace .replace('\n', ' ') for the second case but I can't think of what could be done in the first one. Truncate and ellipsis? What could be a threshold?

Truncate and ellipsis? What could be a threshold?\

I think it's something we can address should we face it. Ideally Truncate and ellipsis sounds like a decent way to clean this up, but it's something we can follow-up on.

EDIT: Just noticed you implemented it 👍 We can fine tune it if needed

BraisGabin

I approved this PR because I thought that the only report using compact() was FindingsReport but I just saw that it also affect the file reports so I don't think we should change the return of compact(). We should find another way to implement this.

BraisGabin · 2022-01-13T22:10:12Z

detekt-report-txt/src/test/kotlin/io/github/detekt/report/txt/TxtOutputReportSpec.kt

-                TestSmellC - [TestEntity] at TestFile.kt:1:1 - Signature=TestEntitySignature
+                TestSmellA - [TestMessage] at TestFile.kt:1:1 - Signature=TestEntitySignature
+                TestSmellB - [TestMessage] at TestFile.kt:1:1 - Signature=TestEntitySignature
+                TestSmellC - [TestMessage] at TestFile.kt:1:1 - Signature=TestEntitySignature


I'm not sure if we should change the txt output too... I'm fine adding behaviour changes in the console report. But I don't feel that confortable changing the file reports. People can be using those structures and have their own parsers so we should keep those.

I changed my mind for the previous reason

gfreivasc · 2022-01-18T15:27:00Z

@BraisGabin your comment makes total sense and I should've found another approach rather than applying these changes elsewhere. Addressed them in the last commit. Will work on the other suggestions yet today.

I also found that I'm changing code on the detekt-api module with these latest changes. Should I do something to the .api file in case we move forward with this approach?

BraisGabin · 2022-01-18T15:39:24Z

Could we don't change Finding at all? I mean, we would lose the "metrics" from the ThresholdedCodeSmell but I think that's ok. The message of the Finding should point out those metrics anyway. And if it doesn't that's a "bug" and we should change it to follow this: https://github.com/detekt/detekt/blob/main/.github/CONTRIBUTING.md#rule-descriptions

gfreivasc · 2022-01-18T15:49:35Z

So in your opinion we should fallback to having a simple extension method similar to the one we had with the previous iteration of DetailedFindingsReport? We could keep metrics that way:

fun Findings.detailed() = when (this) {
    is ThresholdedCodeSmell -> "$id - $metric - [${messageOrDescription()}] at ${location.compact()}"
    else -> "$id - [${messageOrDescription()}] at ${location.compact()}"
}

This would eliminate API impact.

gfreivasc · 2022-01-18T16:51:34Z

detekt-core/src/main/kotlin/io/gitlab/arturbosch/detekt/core/reporting/Reporting.kt

+private fun Finding.truncatedMessage(): String {
+    val message = messageOrDescription()
+        .replace(Regex("\\s+"), " ")
+        .trim()
+    return when {
+        message.length > REPORT_MESSAGE_SIZE_LIMIT -> "${message.take(REPORT_MESSAGE_SIZE_LIMIT)}($ellipsis)"
+        else -> message
+    }
+}


@cortinico I applied this treatment to handle the scenarios you've raised. As seen above, I've loosely set REPORT_MESSAGE_SIZE_LIMIT to be 80. There is a test that validates both truncation and space characters (\n, \r, \t, ...).

BraisGabin · 2022-01-18T17:20:06Z

So in your opinion we should fallback to having a simple extension method similar to the one we had with the previous iteration of DetailedFindingsReport? We could keep metrics that way:
fun Findings.detailed() = when (this) {
    is ThresholdedCodeSmell -> "$id - $metric - [${messageOrDescription()}] at ${location.compact()}"
    else -> "$id - [${messageOrDescription()}] at ${location.compact()}"
}
This would eliminate API impact.

I was thinking about

fun Findings.detailed() = "$id - [${messageOrDescription()}] at ${location.compact()}"
}

But sure, the when have sense too. I'm not sure if $metric is relevant information knowing that the descritption should contain that information too. But I'm fine with both options.

gfreivasc · 2022-01-19T15:42:23Z

About metric it's likely that the information would be redundant, but I think that's not really a problem in terms of presentation. I might be wrong, but I think that could be a change in the future in case that's the conclusion. And an easy change too.

BraisGabin

That have sense. LGTM. Thanks!

Achieved different levels of detailing by changing the format in the implementation of compact() method for CodeSmell and ThresholdedCodeSmell.

Reverts changes to `CodeSmell::compact` method to keep other reports untouched.

Prevents line breaks and long messages from breaking report structure

gfreivasc · 2022-01-26T14:48:03Z

Fixed conflicts related to jUnit 5 migration

cortinico

Change looks go to me 👍 Thanks for doing that. Are we good to merge?

gfreivasc force-pushed the gabrielfv/detailed-findings-report branch from 1b5f523 to 954e741 Compare January 9, 2022 21:18

gfreivasc force-pushed the gabrielfv/detailed-findings-report branch from f9c737f to 21c1de3 Compare January 9, 2022 22:19

BraisGabin previously approved these changes Jan 10, 2022

View reviewed changes

cortinico added notable changes Marker for notable changes in the changelog reports labels Jan 11, 2022

cortinico added this to the 1.20.0 milestone Jan 11, 2022

gfreivasc changed the title ~~Create DetailedFindingsReport ConsoleReport format~~ Adds issue details to findings on FindingsReport and FileBasedFindingsReporter Jan 11, 2022

gfreivasc force-pushed the gabrielfv/detailed-findings-report branch from 21c1de3 to 0a9dd93 Compare January 11, 2022 19:21

cortinico reviewed Jan 12, 2022

View reviewed changes

BraisGabin reviewed Jan 13, 2022

View reviewed changes

gfreivasc force-pushed the gabrielfv/detailed-findings-report branch from 59c6151 to fcdc20e Compare January 18, 2022 15:41

gfreivasc force-pushed the gabrielfv/detailed-findings-report branch 2 times, most recently from ede8fa2 to df3244d Compare January 18, 2022 16:45

gfreivasc commented Jan 18, 2022

View reviewed changes

gfreivasc force-pushed the gabrielfv/detailed-findings-report branch from df3244d to 1b14c2e Compare January 18, 2022 16:52

BraisGabin approved these changes Jan 19, 2022

View reviewed changes

gfreivasc added 4 commits January 26, 2022 11:08

Changes FindingsReporter detail level

8973109

Achieved different levels of detailing by changing the format in the implementation of compact() method for CodeSmell and ThresholdedCodeSmell.

Updates docs with reporting changes

d9382ed

Adapts tests to findings reporter changes

41d7f0b

Creates extension method for console report

b5b96c4

Reverts changes to `CodeSmell::compact` method to keep other reports untouched.

gfreivasc force-pushed the gabrielfv/detailed-findings-report branch from 1b14c2e to 1746b57 Compare January 26, 2022 14:12

Truncates long messages for console reports

b6bee2b

Prevents line breaks and long messages from breaking report structure

gfreivasc force-pushed the gabrielfv/detailed-findings-report branch from 1746b57 to b6bee2b Compare January 26, 2022 14:15

cortinico approved these changes Jan 30, 2022

View reviewed changes

BraisGabin merged commit 0b36ff0 into detekt:main Jan 31, 2022

cortinico mentioned this pull request Jan 31, 2022

Have a reporter that combines the benefits of FindingsReporter and LiteFindintsReporter #4463

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds issue details to findings on FindingsReport and FileBasedFindingsReporter #4464

Adds issue details to findings on FindingsReport and FileBasedFindingsReporter #4464

gfreivasc commented Jan 7, 2022 •

edited

Loading

cortinico commented Jan 7, 2022

codecov bot commented Jan 7, 2022 •

edited

Loading

gfreivasc commented Jan 7, 2022 •

edited

Loading

BraisGabin commented Jan 8, 2022

gfreivasc commented Jan 9, 2022 •

edited

Loading

BraisGabin left a comment

cortinico commented Jan 11, 2022

gfreivasc commented Jan 11, 2022

gfreivasc commented Jan 11, 2022

cortinico Jan 12, 2022

gfreivasc Jan 12, 2022

cortinico Jan 30, 2022 •

edited

Loading

BraisGabin left a comment

BraisGabin Jan 13, 2022

gfreivasc commented Jan 18, 2022 •

edited

Loading

BraisGabin commented Jan 18, 2022

gfreivasc commented Jan 18, 2022

gfreivasc Jan 18, 2022

BraisGabin commented Jan 18, 2022

gfreivasc commented Jan 19, 2022

BraisGabin left a comment •

edited

Loading

gfreivasc commented Jan 26, 2022

cortinico left a comment

Adds issue details to findings on FindingsReport and FileBasedFindingsReporter #4464

Adds issue details to findings on FindingsReport and FileBasedFindingsReporter #4464

Conversation

gfreivasc commented Jan 7, 2022 • edited Loading

Example

LiteFindingsReport

Current FindingsReport

Modified FindingsReport

Observations

cortinico commented Jan 7, 2022

codecov bot commented Jan 7, 2022 • edited Loading

Codecov Report

gfreivasc commented Jan 7, 2022 • edited Loading

BraisGabin commented Jan 8, 2022

gfreivasc commented Jan 9, 2022 • edited Loading

BraisGabin left a comment

Choose a reason for hiding this comment

cortinico commented Jan 11, 2022

gfreivasc commented Jan 11, 2022

gfreivasc commented Jan 11, 2022

cortinico Jan 12, 2022

Choose a reason for hiding this comment

gfreivasc Jan 12, 2022

Choose a reason for hiding this comment

cortinico Jan 30, 2022 • edited Loading

Choose a reason for hiding this comment

BraisGabin left a comment

Choose a reason for hiding this comment

BraisGabin Jan 13, 2022

Choose a reason for hiding this comment

gfreivasc commented Jan 18, 2022 • edited Loading

BraisGabin commented Jan 18, 2022

gfreivasc commented Jan 18, 2022

gfreivasc Jan 18, 2022

Choose a reason for hiding this comment

BraisGabin commented Jan 18, 2022

gfreivasc commented Jan 19, 2022

BraisGabin left a comment • edited Loading

Choose a reason for hiding this comment

gfreivasc commented Jan 26, 2022

cortinico left a comment

Choose a reason for hiding this comment

gfreivasc commented Jan 7, 2022 •

edited

Loading

`LiteFindingsReport`

Current `FindingsReport`

Modified `FindingsReport`

codecov bot commented Jan 7, 2022 •

edited

Loading

gfreivasc commented Jan 7, 2022 •

edited

Loading

gfreivasc commented Jan 9, 2022 •

edited

Loading

cortinico Jan 30, 2022 •

edited

Loading

gfreivasc commented Jan 18, 2022 •

edited

Loading

BraisGabin left a comment •

edited

Loading