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
Move report base path handling to OutputReport #7234
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7234 +/- ##
============================================
- Coverage 84.68% 84.66% -0.03%
- Complexity 3984 3992 +8
============================================
Files 578 578
Lines 12158 12168 +10
Branches 2499 2499
============================================
+ Hits 10296 10302 +6
- Misses 626 627 +1
- Partials 1236 1239 +3 ☔ View full report in Codecov by Sentry. |
@@ -34,4 +34,8 @@ abstract class OutputReport : Extension { | |||
* Defines the translation process of detekt's result into a string. | |||
*/ | |||
abstract fun render(detektion: Detektion): String? | |||
|
|||
companion object { | |||
const val DETEKT_OUTPUT_REPORT_BASE_PATH_KEY = "detekt.output.report.base.path" |
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.
Maybe what I'm talking about is for future PRs but I would like to know what do you think:
- Shouldn't we move this value to a proper property inside the context? Using this constant seems duck tape to avoid break the api. But with 2.0 we can avoid the duck tape.
- If we do that, should we make it nullable? I would vote for no. I think that we can always get good default values on our clients.
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 was in two minds about this. Some reports use absolute paths so I figured it could be better to make it available but not part of the API since it's not used for all reports. But on the other hand it's more convenient to make it part of the API. Could we keep this for a future PR? I don't want to get blocked.
- Agree that base path should not be nullable if it's added to the API as suggested. I've actually found a couple of places where it wasn't being passed through properly which came out in a PR I'm working on now so some of those gaps will be closed and we can make that change.
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.
Could we keep this for a future PR? I don't want to get blocked.
Sure! I'll open an issue :)
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.
Base path is only relevant for report output. This opens the next step of removing FilePath class and using absolute paths everywhere else.