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

Support disabling config validation via tooling spec #4937

Merged
merged 1 commit into from Jun 13, 2022

Conversation

arturbosch
Copy link
Member

@arturbosch arturbosch commented Jun 8, 2022

Disabling config validation is not yet supported in the cli via a flag.

The cli just set the spec value to false and shouldValidateBeforeAnalysis was never evaluated in core.

With this PR tools can programmatically disable config validation if needed.

For a usecase see detekt/detekt-intellij-plugin#179.

@codecov
Copy link

codecov bot commented Jun 8, 2022

Codecov Report

Merging #4937 (49c9cb2) into main (cdf487a) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #4937      +/-   ##
============================================
- Coverage     84.83%   84.83%   -0.01%     
  Complexity     3512     3512              
============================================
  Files           497      497              
  Lines         11549    11554       +5     
  Branches       2139     2141       +2     
============================================
+ Hits           9798     9802       +4     
  Misses          686      686              
- Partials       1065     1066       +1     
Impacted Files Coverage Δ
...ain/kotlin/io/gitlab/arturbosch/detekt/cli/Spec.kt 90.32% <100.00%> (ø)
.../arturbosch/detekt/core/config/ConfigValidators.kt 85.71% <100.00%> (+1.50%) ⬆️
.../io/github/detekt/tooling/dsl/ConfigSpecBuilder.kt 100.00% <100.00%> (ø)
...lin/io/github/detekt/tooling/api/DetektProvider.kt 83.33% <0.00%> (-16.67%) ⬇️

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 cdf487a...49c9cb2. Read the comment docs.

@@ -41,7 +41,7 @@ internal fun CliArgs.createSpec(output: Appendable, error: Appendable): Processi

config {
useDefaultConfig = args.buildUponDefaultConfig
shouldValidateBeforeAnalysis = false
shouldValidateBeforeAnalysis = true
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 better to have 3 values here? Enable/disabled/undefined? This way we don't need to use and. If this is undefined the yml decides. Or we could just remove the yml value and make it only configurable by the cli.

Anyway, this is a step forward so LGTM.

Copy link
Member Author

Choose a reason for hiding this comment

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

Three values would be better yes. That way the code says spec > config.
Yes, removing it from the config would be optimal in a future breaking change.
That is also the case for many other configuration flags we now have in cli and removed from config.

What was the procedure in deprecating config properties? Deprecate it, implement a cli alternative and remove the config property after three minor releases?

I will change change the PR to support three values Enable/disabled/undefined and in a later PR implement the cli flag and deprecate the config property.

Copy link
Member

Choose a reason for hiding this comment

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

To deprecate you just need to add it inside detekt-core/src/main/resources/deprecation.properties. And yes, we should keep it for at least 3 minor versions.

@cortinico cortinico added this to the 1.21.0 milestone Jun 11, 2022
@chao2zhang chao2zhang merged commit 1d47fd3 into main Jun 13, 2022
@chao2zhang chao2zhang deleted the shouldValidate-via-tooling-spec branch June 13, 2022 05:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants