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.

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

Deprecate configuration of reports in detekt Gradle extension #3687

Merged
merged 6 commits into from
Sep 17, 2021

Conversation

3flex
Copy link
Member

@3flex 3flex commented Apr 19, 2021

Reports need to be configured on tasks directly, not by using the detekt extension.

This is to avoid setting the same destination path for multiple detekt reports. An example of this can be seen in the detektTest task of this build scan where the html report is written to the same path as the report generated by the detektMain task because both tasks get the destination path from the extension.

I will add a new section to the Gradle config doc and link to it from the deprecation message on the extension once the PR has been reviewed.

Fixes: #3926
Fixes: #4035
Fixes: #4044
Edit by @cortinico - Added fix issue reference

@codecov
Copy link

codecov bot commented Apr 19, 2021

Codecov Report

Merging #3687 (183eaa4) into main (1a2ca4b) will decrease coverage by 0.00%.
The diff coverage is n/a.

❗ Current head 183eaa4 differs from pull request most recent head 00f8a2a. Consider uploading reports for the commit 00f8a2a to get more accurate results
Impacted file tree graph

@@             Coverage Diff              @@
##               main    #3687      +/-   ##
============================================
- Coverage     83.58%   83.58%   -0.01%     
+ Complexity     3187     3186       -1     
============================================
  Files           459      459              
  Lines          9101     9099       -2     
  Branches       1772     1772              
============================================
- Hits           7607     7605       -2     
  Misses          561      561              
  Partials        933      933              
Impacted Files Coverage Δ
...otlin/io/gitlab/arturbosch/detekt/api/MultiRule.kt 100.00% <0.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 1a2ca4b...00f8a2a. Read the comment docs.

reportId = "CustomJsonReport"
destination = file("build/reports/detekt.json")
}
}
Copy link
Member

Choose a reason for hiding this comment

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

How do I enable/disable some reports now? And should we talk about reportsDir here? Or is it a default in gradle so we don't need to?

Copy link
Member Author

Choose a reason for hiding this comment

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

All report config is done on the task, so you would enable/disable reports either on the individual task level or using withType. That's the doc section I need to write.

reportsDir is used in Gradle for other code quality plugins so we'd now follow that convention. I can mention that in the doc and remove references to the deprecated option (I didn't search to see if there are any yet)

File(extension.reportsDir, compilation.name + ".sarif").absolutePath
}
)
)
Copy link
Member

Choose a reason for hiding this comment

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

would it be possible to extract this to a function/extension function so we should need to pass the name and it will configure all the reports? We have a lot of duplicated code between Multiplatform, JVM and Android (not only this)

Copy link
Member

Choose a reason for hiding this comment

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

And, should we change Plain too?

Copy link
Member Author

@3flex 3flex Apr 21, 2021

Choose a reason for hiding this comment

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

Plain is OK because it defers to the default report path set in the task if the report destination (or now, the outputLocation) is not set (it's an odd exception because the task isn't setup the same way as the others but at least it works):

if (isEnabled) {
val destination = report.destination ?: reportsDir.getOrElse(defaultReportsDir.asFile)
.resolve("${DetektReport.DEFAULT_FILENAME}.${report.type.extension}")
provider.set(destination)

There's a fair bit of refactoring that can be done in the plugin, lots of which is easier once we're on Gradle 6.1. I'd prefer to wait until then before fixing up the repetition.

Comment on lines 84 to 86
@Suppress("DeprecatedCallableAddReplaceWith", "DEPRECATION")
@Deprecated("Customise the reports on the Detekt task(s) instead.", level = DeprecationLevel.WARNING)
fun reports(configure: Action<DetektReports>) = configure.execute(reports)
Copy link
Member

Choose a reason for hiding this comment

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

In the DetektExtension, will it make sense to keep the enabling flags for each report type? My assumptions are:

  • Fewer users are configuring the report output location
  • More users are configuring whether to keep txt/xml/html/sarif.

Copy link
Member Author

Choose a reason for hiding this comment

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

I understand your point, but in Gradle reports are configured on tasks, not on the extension. That's what's done for CodeNarc, Checkstyle and PMD extensions, and that intention is made explicit in the docs:

Things that produce reports (typically tasks) expose a report container that contains Report objects for each possible report that they can produce.

reference

We can't implement ReportContainer directly but in keeping with that convention (and slightly simplifying the plugin) I still think it should be removed.

@chao2zhang chao2zhang added this to the 1.17.0 milestone Apr 26, 2021
@3flex
Copy link
Member Author

3flex commented May 5, 2021

Any other concerns on this before merging? I'd only suggest waiting until after 1.17 release now some RCs have been published.

@BraisGabin BraisGabin removed this from the 1.17.0 milestone May 5, 2021
@BraisGabin
Copy link
Member

Agree, we should wait until 1.17.0 is released.

@3flex 3flex added this to the 1.18 milestone May 14, 2021
@cortinico cortinico added gradle-plugin notable changes Marker for notable changes in the changelog labels May 20, 2021
@3flex
Copy link
Member Author

3flex commented May 25, 2021

LGTM?

Copy link
Member

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

Can we please extend a bit on the documentation for this PR?

Specifically we're deprecating a property that is suggested to use in our README and in the homepage of the website: https://detekt.github.io/detekt/ Therefore we should provide clear alternatives to our users.

Potentially also a migration page should be added as well.

@cortinico
Copy link
Member

Should we push this to 1.19.0?

@3flex
Copy link
Member Author

3flex commented Jul 24, 2021

Yeah sorry I haven't had time to dedicate to this recently. Happy to push it back.

I'll need to rebase and still write some docs.

@3flex
Copy link
Member Author

3flex commented Aug 21, 2021

Rebased & docs updated. I believe I've updated all sections where the deprecated configuration was used, except the old "News" posts.

Migration instructions were added to the changelog.

Copy link
Member

@BraisGabin BraisGabin left a comment

Choose a reason for hiding this comment

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

I'd vote to split this PR in two. One with code and chancellor and the other with the doc. Otherwise the users will see a "too updated" documentation untilnwe release 1.19

3flex added a commit to 3flex/detekt that referenced this pull request Aug 21, 2021
@3flex
Copy link
Member Author

3flex commented Aug 21, 2021

Time for versioned docs? :)

I've split them up - doc updates in #4064. I've left the changelog & migration update in this PR because the changelog is versioned so it's clear that it's related to unreleased changes.

@BraisGabin
Copy link
Member

Is there something missing here? Can we merge this? This is a big change fot 1.19.0 so I think that as soon as we merge it as better. Someone using the SNAPSHOT can find some issue or we can find some issue while developing the tool itself.

@cortinico
Copy link
Member

☝️ Agree. I believe we're good to go for merging this

@3flex 3flex merged commit bbbbd25 into detekt:main Sep 17, 2021
@3flex 3flex deleted the reports-dsl branch September 17, 2021 01:17
3flex added a commit to 3flex/detekt that referenced this pull request Nov 29, 2021
Doc update for detekt#3687

# Conflicts:
#	docs/pages/gettingstarted/gradle.md
BraisGabin pushed a commit that referenced this pull request Nov 29, 2021
Doc update for #3687

# Conflicts:
#	docs/pages/gettingstarted/gradle.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gradle-plugin notable changes Marker for notable changes in the changelog
Projects
None yet
4 participants