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.

Sign up for GitHub

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

Jump to bottom

Move report base path handling to OutputReport #7234

Merged
merged 2 commits into from Apr 28, 2024
Merged

Move report base path handling to OutputReport #7234

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
86b5741
Move report base path handling to OutputReport
3flex Apr 27, 2024
25aa867
Use OutputReport project base path to handle relative path output
3flex Apr 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions detekt-api/api/detekt-api.api
View file
Open in desktop
Expand Up @@ -231,6 +231,8 @@ public final class io/gitlab/arturbosch/detekt/api/Notification$Level : java/lan
}

public abstract class io/gitlab/arturbosch/detekt/api/OutputReport : io/gitlab/arturbosch/detekt/api/Extension {
public static final field Companion Lio/gitlab/arturbosch/detekt/api/OutputReport$Companion;
public static final field DETEKT_OUTPUT_REPORT_BASE_PATH_KEY Ljava/lang/String;
public fun <init> ()V
public abstract fun getEnding ()Ljava/lang/String;
public fun getPriority ()I
Expand All @@ -240,6 +242,9 @@ public abstract class io/gitlab/arturbosch/detekt/api/OutputReport : io/gitlab/a
public final fun write (Ljava/nio/file/Path;Lio/gitlab/arturbosch/detekt/api/Detektion;)V
}

public final class io/gitlab/arturbosch/detekt/api/OutputReport$Companion {
}

public class io/gitlab/arturbosch/detekt/api/ProjectMetric {
public fun <init> (Ljava/lang/String;IIZI)V
public synthetic fun <init> (Ljava/lang/String;IIZIILkotlin/jvm/internal/DefaultConstructorMarker;)V
Expand Down
4 changes: 4 additions & 0 deletions detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/OutputReport.kt
View file
Open in desktop
Expand Up @@ -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"
Copy link
Member

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:

  1. 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.
  2. 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.

Copy link
Member Author

@3flex 3flex Apr 27, 2024 •
edited

Choose a reason for hiding this comment

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

  1. 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.
  2. 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.

Copy link
Member

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 :)

Copy link
Member

Choose a reason for hiding this comment

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

#7241

}
}
1 change: 0 additions & 1 deletion detekt-api/src/testFixtures/kotlin/io/gitlab/arturbosch/detekt/test/TestFactory.kt
View file
Open in desktop
Expand Up @@ -71,7 +71,6 @@ fun createIssueForRelativePath(
basePath: String = "Users/tester/detekt/",
relativePath: String = "TestFile.kt"
): Issue {
require(!basePath.startsWith("/")) { "The path shouldn't start with '/'" }
return IssueImpl(
ruleInfo = ruleInfo,
entity = Entity(
Expand Down
16 changes: 15 additions & 1 deletion detekt-core/src/test/kotlin/io/gitlab/arturbosch/detekt/core/reporting/OutputFacadeSpec.kt
View file
Open in desktop
Expand Up @@ -10,7 +10,9 @@ import io.github.detekt.test.utils.resourceAsPath
import io.gitlab.arturbosch.detekt.core.DetektResult
import io.gitlab.arturbosch.detekt.core.createNullLoggingSpec
import io.gitlab.arturbosch.detekt.core.tooling.withSettings
import io.gitlab.arturbosch.detekt.test.createEntity
import io.gitlab.arturbosch.detekt.test.createIssue
import io.gitlab.arturbosch.detekt.test.createLocation
import io.gitlab.arturbosch.detekt.test.createRuleInfo
import org.assertj.core.api.Assertions.assertThat
import org.junit.jupiter.api.Test
Expand All @@ -22,7 +24,19 @@ class OutputFacadeSpec {
fun `Running the output facade with multiple reports`() {
val printStream = StringPrintStream()
val inputPath: Path = resourceAsPath("/cases")
val defaultResult = DetektResult(listOf(createIssue(createRuleInfo(ruleSetId = "Key"))))
val defaultResult = DetektResult(
listOf(
createIssue(
createRuleInfo(ruleSetId = "Key"),
createEntity(
location = createLocation(
"TestFile.kt",
System.getProperty("user.dir")
)
)
)
)
)
val plainOutputPath = createTempFileForTest("detekt", ".txt")
val htmlOutputPath = createTempFileForTest("detekt", ".html")
val xmlOutputPath = createTempFileForTest("detekt", ".xml")
Expand Down
14 changes: 13 additions & 1 deletion detekt-report-html/src/main/kotlin/io/github/detekt/report/html/HtmlOutputReport.kt
View file
Open in desktop
Expand Up @@ -7,7 +7,9 @@ import io.gitlab.arturbosch.detekt.api.Issue
import io.gitlab.arturbosch.detekt.api.OutputReport
import io.gitlab.arturbosch.detekt.api.ProjectMetric
import io.gitlab.arturbosch.detekt.api.RuleSet
import io.gitlab.arturbosch.detekt.api.SetupContext
import io.gitlab.arturbosch.detekt.api.TextLocation
import io.gitlab.arturbosch.detekt.api.getOrNull
import io.gitlab.arturbosch.detekt.api.internal.BuiltInOutputReport
import io.gitlab.arturbosch.detekt.api.internal.whichDetekt
import kotlinx.html.CommonAttributeGroupFacadeFlowInteractiveContent
Expand All @@ -27,11 +29,14 @@ import kotlinx.html.span
import kotlinx.html.stream.createHTML
import kotlinx.html.ul
import kotlinx.html.visit
import java.nio.file.Path
import java.time.OffsetDateTime
import java.time.ZoneOffset
import java.time.format.DateTimeFormatter
import java.util.Locale
import kotlin.io.path.absolute
import kotlin.io.path.invariantSeparatorsPathString
import kotlin.io.path.relativeTo

private const val DEFAULT_TEMPLATE = "default-html-report-template.html"
private const val PLACEHOLDER_METRICS = "@@@metrics@@@"
Expand All @@ -51,6 +56,12 @@ class HtmlOutputReport : BuiltInOutputReport, OutputReport() {
override val id: String = "HtmlOutputReport"
override val ending = "html"

var basePath: Path? = null

override fun init(context: SetupContext) {
basePath = context.getOrNull<Path>(DETEKT_OUTPUT_REPORT_BASE_PATH_KEY)?.absolute()
}

override fun render(detektion: Detektion) =
javaClass.getResource("/$DEFAULT_TEMPLATE")!!
.openSafeStream()
Expand Down Expand Up @@ -146,7 +157,8 @@ class HtmlOutputReport : BuiltInOutputReport, OutputReport() {
}

private fun FlowContent.renderIssue(issue: Issue) {
val filePath = issue.location.filePath.relativePath ?: issue.location.filePath.absolutePath
val filePath = basePath?.let { issue.location.filePath.absolutePath.relativeTo(it) }
?: issue.location.filePath.absolutePath
val pathString = filePath.invariantSeparatorsPathString
span("location") {
text(
Expand Down
10 changes: 7 additions & 3 deletions detekt-report-html/src/test/kotlin/io/github/detekt/report/html/HtmlOutputReportSpec.kt
View file
Open in desktop
Expand Up @@ -82,6 +82,9 @@ class HtmlOutputReportSpec {

@Test
fun `renders the right file locations for relative paths`() {
val htmlReport = HtmlOutputReport()
htmlReport.basePath = Path("Users/tester/detekt/").absolute()

val result = htmlReport.render(createTestDetektionFromRelativePath())

assertThat(result).contains("<span class=\"location\">src/main/com/sample/Sample1.kt:11:1</span>")
Expand Down Expand Up @@ -208,10 +211,11 @@ private fun createTestDetektionWithMultipleSmells(): Detektion {
}

private fun createTestDetektionFromRelativePath(): Detektion {
val basePath = "${System.getProperty("user.dir")}/Users/tester/detekt/"
val entity1 = createEntity(
location = createLocation(
path = "src/main/com/sample/Sample1.kt",
basePath = "Users/tester/detekt/",
basePath = basePath,
position = 11 to 1,
text = 10..14,
),
Expand All @@ -220,14 +224,14 @@ private fun createTestDetektionFromRelativePath(): Detektion {
val entity2 = createEntity(
location = createLocation(
path = "src/main/com/sample/Sample2.kt",
basePath = "Users/tester/detekt/",
basePath = basePath,
position = 22 to 2,
)
)
val entity3 = createEntity(
location = createLocation(
path = "src/main/com/sample/Sample3.kt",
basePath = "Users/tester/detekt/",
basePath = basePath,
position = 33 to 3,
)
)
Expand Down
30 changes: 21 additions & 9 deletions detekt-report-md/src/main/kotlin/io/github/detekt/report/md/MdOutputReport.kt
View file
Open in desktop
Expand Up @@ -15,14 +15,19 @@
import io.gitlab.arturbosch.detekt.api.Issue
import io.gitlab.arturbosch.detekt.api.OutputReport
import io.gitlab.arturbosch.detekt.api.ProjectMetric
import io.gitlab.arturbosch.detekt.api.SetupContext
import io.gitlab.arturbosch.detekt.api.SourceLocation
import io.gitlab.arturbosch.detekt.api.getOrNull
import io.gitlab.arturbosch.detekt.api.internal.BuiltInOutputReport
import io.gitlab.arturbosch.detekt.api.internal.whichDetekt
import java.nio.file.Path
import java.time.OffsetDateTime
import java.time.ZoneOffset
import java.time.format.DateTimeFormatter
import java.util.Locale
import kotlin.io.path.absolute
import kotlin.io.path.invariantSeparatorsPathString
import kotlin.io.path.relativeTo
import kotlin.math.max
import kotlin.math.min

Expand All @@ -39,6 +44,12 @@
override val id: String = "MdOutputReport"
override val ending: String = "md"

var basePath: Path? = null

Check warning on line 47 in detekt-report-md/src/main/kotlin/io/github/detekt/report/md/MdOutputReport.kt

View check run for this annotation

Codecov / codecov/patch

detekt-report-md/src/main/kotlin/io/github/detekt/report/md/MdOutputReport.kt#L47 

Added line #L47 was not covered by tests

override fun init(context: SetupContext) {
basePath = context.getOrNull<Path>(DETEKT_OUTPUT_REPORT_BASE_PATH_KEY)?.absolute()
}

override fun render(detektion: Detektion) = markdown {
h1 { "detekt" }

Expand All @@ -48,7 +59,7 @@
h2 { "Complexity Report" }
renderComplexity(getComplexityMetrics(detektion))

renderIssues(detektion.issues)
renderIssues(detektion.issues, basePath)
emptyLine()

paragraph {
Expand Down Expand Up @@ -81,17 +92,17 @@
}
}

private fun MarkdownContent.renderGroup(issues: List<Issue>) {
private fun MarkdownContent.renderGroup(issues: List<Issue>, basePath: Path?) {
issues
.groupBy { it.ruleInfo }
.toList()
.sortedBy { (ruleInfo, _) -> ruleInfo.id.value }
.forEach { (ruleInfo, ruleIssues) ->
renderRule(ruleInfo, ruleIssues)
renderRule(ruleInfo, ruleIssues, basePath)
}
}

private fun MarkdownContent.renderRule(ruleInfo: Issue.RuleInfo, issues: List<Issue>) {
private fun MarkdownContent.renderRule(ruleInfo: Issue.RuleInfo, issues: List<Issue>, basePath: Path?) {
val ruleId = ruleInfo.id.value
val ruleSetId = ruleInfo.ruleSetId.value
h3 { "$ruleSetId, $ruleId (%,d)".format(Locale.ROOT, issues.size) }
Expand All @@ -114,12 +125,12 @@
)
)
.forEach {
item { renderIssue(it) }
item { renderIssue(it, basePath) }
}
}
}

private fun MarkdownContent.renderIssues(issues: List<Issue>) {
private fun MarkdownContent.renderIssues(issues: List<Issue>, basePath: Path?) {
val total = issues.count()

h2 { "Issues (%,d)".format(Locale.ROOT, total) }
Expand All @@ -129,12 +140,13 @@
.toList()
.sortedBy { (group, _) -> group.value }
.forEach { (_, groupIssues) ->
renderGroup(groupIssues)
renderGroup(groupIssues, basePath)
}
}

private fun MarkdownContent.renderIssue(issue: Issue): String {
val filePath = issue.location.filePath.relativePath ?: issue.location.filePath.absolutePath
private fun MarkdownContent.renderIssue(issue: Issue, basePath: Path?): String {
val filePath = basePath?.let { issue.location.filePath.absolutePath.relativeTo(it) }
?: issue.location.filePath.absolutePath
val location =
"${filePath.invariantSeparatorsPathString}:${issue.location.source.line}:${issue.location.source.column}"

Expand Down
1 change: 0 additions & 1 deletion detekt-report-sarif/src/main/kotlin/io/github/detekt/report/sarif/SarifOutputReport.kt
View file
Open in desktop
Expand Up @@ -19,7 +19,6 @@ import kotlin.io.path.Path
import kotlin.io.path.absolute
import kotlin.io.path.invariantSeparatorsPathString

const val DETEKT_OUTPUT_REPORT_BASE_PATH_KEY = "detekt.output.report.base.path"
const val SRCROOT = "%SRCROOT%"

class SarifOutputReport : BuiltInOutputReport, OutputReport() {
Expand Down
1 change: 1 addition & 0 deletions detekt-report-sarif/src/test/kotlin/io/github/detekt/report/sarif/SarifOutputReportSpec.kt
View file
Open in desktop
Expand Up @@ -4,6 +4,7 @@ import io.github.detekt.test.utils.readResourceContent
import io.gitlab.arturbosch.detekt.api.CodeSmell
import io.gitlab.arturbosch.detekt.api.Config
import io.gitlab.arturbosch.detekt.api.Entity
import io.gitlab.arturbosch.detekt.api.OutputReport.Companion.DETEKT_OUTPUT_REPORT_BASE_PATH_KEY
import io.gitlab.arturbosch.detekt.api.Rule
import io.gitlab.arturbosch.detekt.api.RuleSet
import io.gitlab.arturbosch.detekt.api.RuleSetProvider
Expand Down
16 changes: 15 additions & 1 deletion detekt-report-xml/src/main/kotlin/io/github/detekt/report/xml/XmlOutputReport.kt
View file
Open in desktop
Expand Up @@ -3,9 +3,14 @@ package io.github.detekt.report.xml
import io.gitlab.arturbosch.detekt.api.Detektion
import io.gitlab.arturbosch.detekt.api.Issue
import io.gitlab.arturbosch.detekt.api.OutputReport
import io.gitlab.arturbosch.detekt.api.SetupContext
import io.gitlab.arturbosch.detekt.api.getOrNull
import io.gitlab.arturbosch.detekt.api.internal.BuiltInOutputReport
import java.nio.file.Path
import java.util.Locale
import kotlin.io.path.absolute
import kotlin.io.path.invariantSeparatorsPathString
import kotlin.io.path.relativeTo

/**
* Contains rule violations in an XML format. The report follows the structure of a Checkstyle report.
Expand All @@ -19,13 +24,22 @@ class XmlOutputReport : BuiltInOutputReport, OutputReport() {
private val Issue.severityLabel: String
get() = severity.name.lowercase(Locale.US)

var basePath: Path? = null

override fun init(context: SetupContext) {
basePath = context.getOrNull<Path>(DETEKT_OUTPUT_REPORT_BASE_PATH_KEY)?.absolute()
}

override fun render(detektion: Detektion): String {
val lines = ArrayList<String>()
lines += "<?xml version=\"1.0\" encoding=\"UTF-8\"?>"
lines += "<checkstyle version=\"4.3\">"

detektion.issues
.groupBy { it.location.filePath.relativePath ?: it.location.filePath.absolutePath }
.groupBy {
basePath?.let { path -> it.location.filePath.absolutePath.relativeTo(path) }
?: it.location.filePath.absolutePath
}
.forEach { (filePath, issues) ->
lines += "<file name=\"${filePath.invariantSeparatorsPathString.toXmlString()}\">"
issues.forEach {
Expand Down
7 changes: 7 additions & 0 deletions detekt-report-xml/src/test/kotlin/io/github/detekt/report/xml/XmlOutputFormatSpec.kt
View file
Open in desktop
Expand Up @@ -13,6 +13,8 @@ import org.junit.jupiter.api.Test
import org.junit.jupiter.params.ParameterizedTest
import org.junit.jupiter.params.provider.EnumSource
import java.util.Locale
import kotlin.io.path.Path
import kotlin.io.path.absolute
import kotlin.io.path.invariantSeparatorsPathString

private const val TAB = "\t"
Expand Down Expand Up @@ -114,13 +116,18 @@ class XmlOutputFormatSpec {
fun `renders issues with relative path`() {
val issueA = createIssueForRelativePath(
ruleInfo = createRuleInfo("id_a"),
basePath = "${System.getProperty("user.dir")}/Users/tester/detekt/",
relativePath = "Sample1.kt"
)
val issueB = createIssueForRelativePath(
ruleInfo = createRuleInfo("id_b"),
basePath = "${System.getProperty("user.dir")}/Users/tester/detekt/",
relativePath = "Sample2.kt"
)

val outputFormat = XmlOutputReport()
outputFormat.basePath = Path("Users/tester/detekt/").absolute()

val result = outputFormat.render(TestDetektion(issueA, issueB))

assertThat(result).isEqualTo(
Expand Down