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

Add config generator for custom rules #5080

Merged
merged 9 commits into from Sep 12, 2022

Conversation

VitalyVPinchuk
Copy link
Contributor

@VitalyVPinchuk VitalyVPinchuk commented Jul 15, 2022

Work in progress
For #4457

This is just a draft to understand if I'm doing it the right way.
Please, share your thoughts.

Created gradle task generateCustomConfig by copying generateDocumentation which accepts source path for rules and creates config.yml in the path indicated in config parameter.

@codecov
Copy link

codecov bot commented Jul 15, 2022

Codecov Report

Merging #5080 (991d229) into main (95624c3) will not change coverage.
The diff coverage is n/a.

Current head 991d229 differs from pull request most recent head 427a949. Consider uploading reports for the commit 427a949 to get more accurate results

@@     Coverage Diff      @@
##   main   #5080   +/-   ##
============================
============================

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@VitalyVPinchuk VitalyVPinchuk marked this pull request as draft July 15, 2022 18:41
@cortinico
Copy link
Member

This is just a draft to understand if I'm doing it the right way.
Please, share your thoughts.

Thanks for doing this.
I think we'll need a bit of re-work. Specifically, my understanding of the problem is:

  • We want to expose a custom task to rule authors. So we'll have to create it inside the detekt-gradle-plugin.
  • I would call the task differently. Like detektGenerateCustomRuleConfig or so.
  • What the task should do is: output a config.yml inside the /resources folder of a custom project, or fail if there is no custom ruleset or so.

@BraisGabin
Copy link
Collaborator

I agree with @cortinico but should we include it in our current gradle plugin? Or should we create a different one? 99.9% of our users will not use this action.

@cortinico
Copy link
Member

but should we include it in our current gradle plugin?

Yes it should be inside the plugin, but as a separate Task. Ideally we should not register the task by default, but let users do so if they wish

@VitalyVPinchuk
Copy link
Contributor Author

Hello everyone!
I ran into problems and need help, please.

What I'm trying to do is to call a function in detekt-generator module (for example, io.gitlab.arturbosch.detekt.generator.Main.main()) from detekt-gradle-plugin package.
So I need a dependency to detekt-generator.

  • In DetektPlugin.kt:82 I added dependencySet.add(project.dependencies.create("io.gitlab.arturbosch.detekt:detekt-generator:$version")). And when I try to load class in DetektInvoker.kt:52 by calling loader.loadClass("io.gitlab.arturbosch.detekt.generator.Main") the "Class not found exception" occurs. But calling loader.loadClass("io.gitlab.arturbosch.detekt.cli.Main") works fine even though I delete dependencySet.add(project.dependencies.create("io.gitlab.arturbosch.detekt:detekt-cli:$version"))! How is that even possible???
    And isn't Gradle trying to get the dependency from network repository? Because it requests version 1.20.0 whereas local version is 1.21.0.-RC2.

  • I tried to includeBuild("../detekt-generator") in build.gradle.kts. But in this case applying plugin id("module") leads to error "module not found". Why is it so? And the other error is "name detekt-generator already used in main build". I don't see where the duplicate is.

  • The only way I managed to do this is to add dependency directly from local binaries like this: implementation(fileTree("../detekt-generator/build/libs/") { include("*.jar") }). But I think it's not an option because it will require to manually build detekt-generator prior to building detekt-gradle-plugin.

@cortinico
Copy link
Member

Hey @VitalyVPinchuk 👋 Happy to follow up.

What I'm trying to do is to call a function in detekt-generator module

I would suggest against doing this ☝️

Which function specifically are you looking into copying?
You should pass through the detekt-cli OR copy over the code you need inside the Gradle Plugin.

The reason you can't depend on detekt-generator is that detekt-gradle-plugin is an included build. It's fully isolated from the other packages and should remain so in order for better testing and project isolation.

@VitalyVPinchuk
Copy link
Contributor Author

Hi @cortinico

I would suggest against doing this ☝️

Okay, I got it, I won't!

You should pass through the detekt-cli OR copy over the code you need inside the Gradle Plugin.

How do I pass through the detekt-cli? Is there a sample code?

The reason you can't depend on detekt-generator is that detekt-gradle-plugin is an included build. It's fully isolated from the other packages and should remain so in order for better testing and project isolation.

Okay, we'll leave it as it is :) No dependencies.
But isn't it technically possible? I created for test purposes a standalone build inside detekt (like build-logic or detekt-gradle-plugin) and was able to includeBuild("../my-module") in detekt-gradle-plugin/settings.gradle.kts and call its functions from DetektPlugin.kt.

@cortinico
Copy link
Member

How do I pass through the detekt-cli? Is there a sample code?

I'm on mobile now, but essentially what the detetkGenerate task is already doing. I would need to look furhter into this as it could be non trivial 🤔

@VitalyVPinchuk
Copy link
Contributor Author

OMG!
Something went wrong with my squash 🤔

@BraisGabin
Copy link
Collaborator

I like the idea. If we have a cli in the future we can iterate and wrap it inside a gradle plugin or whatever we want. It will not be the more published solution but it's a tool that solves the problem.

@VitalyVPinchuk
Copy link
Contributor Author

Hi!
I reverted unnecessary changes and added a test.

Copy link
Collaborator

@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.

LGTM :) Thanks for this work :)

@BraisGabin
Copy link
Collaborator

It seems that the fails on Windows are "legit". Could you take a look?

@VitalyVPinchuk
Copy link
Contributor Author

It seems that the fails on Windows are "legit". Could you take a look?

I've tried to fix it but Windows still can't find file D:\a\detekt\detekt\detekt-rules-complexity\src\main\resources\config\config.yml
What could be the problem?

@BraisGabin BraisGabin added this to the 1.22.0 milestone Aug 17, 2022
Co-authored-by: Brais Gabín <braisgabin@gmail.com>
@VitalyVPinchuk
Copy link
Contributor Author

@BraisGabin thanks, it works!
Now I understand what the problem was.

@cortinico cortinico changed the title [WIP] Add config generator for custom rules Add config generator for custom rules Sep 7, 2022
@cortinico cortinico added rules notable changes Marker for notable changes in the changelog and removed rules labels Sep 7, 2022
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.

Sorry for the late review, I think we're fine with this and we can merge it 🎉 Sorry for the back and forth but it was also a bit unclear for us how we wanted this feature to look like.

Last thing to do (as a follow-up) would be to write a short paragraph on the website about this.

@BraisGabin BraisGabin merged commit 1c3e62b into detekt:main Sep 12, 2022
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notable changes Marker for notable changes in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants