-
-
Notifications
You must be signed in to change notification settings - Fork 779
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
Number format in some report #2289
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2289 +/- ##
============================================
+ Coverage 81.69% 81.71% +0.01%
Complexity 2111 2111
============================================
Files 348 348
Lines 6038 6039 +1
Branches 1105 1105
============================================
+ Hits 4933 4935 +2
Misses 519 519
+ Partials 586 585 -1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is fine. I like it, because we align the format of the different reports.
@@ -14,21 +15,19 @@ class ComplexityReportGenerator(private val complexityMetric: ComplexityMetric) | |||
ComplexityReportGenerator(ComplexityMetric(detektion)) | |||
} | |||
|
|||
fun generate(): String? { | |||
fun generate(): List<String>? { | |||
if (cannotGenerate()) return null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An empty list makes more sense here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about this. The null says: there's not report. An empty list sais: there is a report and it's empty. In the html report both are the same but in the console report don't. Because we need to add the header. We could check if the list is empty but I think that it's not that clean.
val complexities = complexityReport.split("\n") | ||
return complexities.subList(1, complexities.size - 1) | ||
} | ||
return ComplexityReportGenerator.create(detektion).generate() ?: emptyList() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If generate()
returns an empty list you won't need this conditional statement anymore.
Maybe this method can then be inlined as well.
This fixes the dashes in the html report in the complexity section
Add thousand separators. Remove useless
-
and remove space between value and %This is before:
and this is after this PR:
This changes are made for the console output too. If we don't want them I can refactorice them