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

Validate yaml configurations by comparing their structure - #516 #1998

Merged
merged 10 commits into from
Nov 3, 2019

Conversation

arturbosch
Copy link
Member

@arturbosch arturbosch commented Oct 8, 2019

The base idea is this function signature: fun verifyConfig(config: Config, baseline: Config): List<Notification>

We take the user config and the default detekt config and compare their structure.
My idea to integrate this into CLI is via a config block:

config:
    active: false // by default? no performance tested yet, should be negligible
    excludes: user-ruleset, user-property

This way we can exclude user rules. Shouldn't be to much of a hassle due to rule set grouping.

The reporting mechanism tells the user that this property does not exist.
We could integrate a string distance comparison on each "sub config" level and suggest possible options. The performance should be tested here as for example the Levenstheins Distance could be quite expensive.

@arturbosch arturbosch added this to the 1.2.0 milestone Oct 8, 2019
@3flex
Copy link
Member

3flex commented Oct 29, 2019

There are requests to support multiple user config files (instead of limited to default detekt config and a single user config file). I think this should be considered when implementing this change.

@arturbosch
Copy link
Member Author

arturbosch commented Oct 30, 2019

There are requests to support multiple user config files (instead of limited to default detekt config and a single user config file). I think this should be considered when implementing this change.

Can you elaborate? Do you mean CompositeConfig aka when using --config config1,config2?
If yes, I just pushed a commit to support them.

Have even found two errors in our detekt.yml with this feature :):

Property 'formatting>NoItParamInMultilineLambda' is misspelled or does not exist.
Property 'style>VariableNaming' is misspelled or does not exist.

@3flex
Copy link
Member

3flex commented Oct 31, 2019

Can you elaborate?

See #2045. I believe that's not supported today? So instead of validating only two files the validation should support validation of a merged config, where that merged config could be made up of 1..n files instead of 1..2 like is supported today.

@arturbosch arturbosch changed the title [WIP] Validate yaml configurations by comparing their structure - #516 Validate yaml configurations by comparing their structure - #516 Nov 1, 2019
@arturbosch
Copy link
Member Author

Can you elaborate?

See #2045. I believe that's not supported today? So instead of validating only two files the validation should support validation of a merged config, where that merged config could be made up of 1..n files instead of 1..2 like is supported today.

Yep, this is supported via the CompositeConfig which this PR also validates. #2045 sounds like a regression to me which does not need special treatment here.

To me this PR is ready to be reviewed :)

@schalkms
Copy link
Member

schalkms commented Nov 2, 2019

Documentation seems to be outdated. That's the reason for the failing build.

@codecov-io
Copy link

codecov-io commented Nov 2, 2019

Codecov Report

Merging #1998 into master will decrease coverage by 0.1%.
The diff coverage is 71.62%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1998      +/-   ##
============================================
- Coverage     80.59%   80.49%   -0.11%     
- Complexity     1988     2001      +13     
============================================
  Files           332      336       +4     
  Lines          5705     5767      +62     
  Branches       1042     1060      +18     
============================================
+ Hits           4598     4642      +44     
- Misses          554      565      +11     
- Partials        553      560       +7
Impacted Files Coverage Δ Complexity Δ
...gitlab/arturbosch/detekt/cli/ArgumentConverters.kt 72% <ø> (ø) 0 <0> (?)
...ain/kotlin/io/gitlab/arturbosch/detekt/cli/Main.kt 0% <0%> (ø) 0 <0> (ø) ⬇️
...kotlin/io/gitlab/arturbosch/detekt/cli/ExitCode.kt 0% <0%> (ø) 0 <0> (?)
...in/io/gitlab/arturbosch/detekt/api/SplitPattern.kt 78.94% <100%> (ø) 17 <1> (ø) ⬇️
...io/gitlab/arturbosch/detekt/api/CompositeConfig.kt 66.66% <100%> (+6.66%) 5 <2> (+1) ⬆️
...n/io/gitlab/arturbosch/detekt/cli/InvalidConfig.kt 100% <100%> (ø) 1 <1> (?)
...n/kotlin/io/gitlab/arturbosch/detekt/api/Config.kt 91.66% <100%> (ø) 0 <0> (ø) ⬇️
...ekt/generator/printer/rulesetpage/ConfigPrinter.kt 75% <100%> (+2.02%) 8 <1> (+1) ⬆️
...bosch/detekt/api/internal/CommaSeparatedPattern.kt 100% <100%> (ø) 2 <2> (?)
...tlin/io/gitlab/arturbosch/detekt/api/YamlConfig.kt 82.6% <50%> (-3.76%) 8 <0> (ø)
... and 15 more

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 72f2225...4c583a0. Read the comment docs.

@arturbosch arturbosch merged commit 6c37955 into master Nov 3, 2019
@arturbosch arturbosch deleted the validate-configs branch November 3, 2019 09:11
smyachenkov pushed a commit to smyachenkov/detekt that referenced this pull request Dec 9, 2019
detekt#1998)

* Validate yaml configurations by comparing their structure - detekt#516

* Towards supporting composite config validation

* Generalize config validation interface

* Fix formatting

* Delete non existing rule configurations

* Support excluding config properties from validation

* Use settings#info to print invalid properties

* Exclude common rule properties by default from validation

* Regenerate default configuration

* Rename validation message functions
smyachenkov pushed a commit to smyachenkov/detekt that referenced this pull request Dec 9, 2019
detekt#1998)

* Validate yaml configurations by comparing their structure - detekt#516

* Towards supporting composite config validation

* Generalize config validation interface

* Fix formatting

* Delete non existing rule configurations

* Support excluding config properties from validation

* Use settings#info to print invalid properties

* Exclude common rule properties by default from validation

* Regenerate default configuration

* Rename validation message functions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants