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

Allow users and rule authors to specify a reason per configured value #4611

Merged
merged 28 commits into from Jun 1, 2022

Conversation

marschwar
Copy link
Contributor

@marschwar marschwar commented Mar 6, 2022

What does this PR address?
A few times there was the request to be able to specify a custom error message or a reason for certain setting. See #3501 for example. This will allow rule authors to enable this features for all rules that have configurable list of string values such as

  ForbiddenImport:
    active: true
    imports:
      - value: 'org.junit.Test'
        reason: 'Do not use JUnit4. Replace with org.junit.jupiter.api.Test.'

This is only providing the option to do so and does not change any rules.

Thanks to @BraisGabin for collaborating on this.

Copy link
Collaborator

@BraisGabin BraisGabin left a comment

* I do not really like the nameof the `ExplainedValues` type and `explainedValues` config delegate. Any idea what we could call the combination of a configured value and the reasoning why the value is configured?

ValueWithReason? I don't think it's better... it's the only other option that I find.

* Do you agree on the representation in the `config.yml` (see `detekt-core/src/test/resources/explained-values.yml`)?

I like it.

* Should the reason be shown in the documentation? If so, any ideas on how to do that? (This would be a separate PR)

I don't think so. I don't think that it could provide any value for the reader.

@marschwar
Copy link
Contributor Author

marschwar commented Mar 7, 2022

I think I also prefer ValueWithReason but before changing everything multiple times I would like to wait on some more opinions.

@cortinico
Copy link
Member

cortinico commented Mar 10, 2022

I haven't looked into the code, but if I understand correctly it relies on a separate explained-values.yml file right?

I would rather move this file inside the config on a top level explanation: key or even move it closer to the rule config as a metadata addition

@marschwar
Copy link
Contributor Author

marschwar commented Mar 17, 2022

if I understand correctly it relies on a separate explained-values.yml file right?

No, that is not correct the explained-values.yml is only used in the tests to verify that the config parser can read those formats correctly. The idea is, that rule authors may decide that their rule supports values with reasons instead of values only. So instead of

  ForbiddenImport:
    active: true
    imports:
      - 'org.assertj.core.api.Assertions'

it could become

  ForbiddenImport:
    active: true
    imports:
      - value: 'org.assertj.core.api.Assertions'
        reason: 'import Assertions.assertThat directly'

@codecov
Copy link

codecov bot commented Mar 21, 2022

Codecov Report

Merging #4611 (b27a563) into main (a8cde4f) will decrease coverage by 0.07%.
The diff coverage is 77.06%.

@@             Coverage Diff              @@
##               main    #4611      +/-   ##
============================================
- Coverage     84.80%   84.73%   -0.08%     
  Complexity     3453     3453              
============================================
  Files           492      493       +1     
  Lines         11328    11397      +69     
  Branches       2086     2103      +17     
============================================
+ Hits           9607     9657      +50     
- Misses          673      682       +9     
- Partials       1048     1058      +10     
Impacted Files Coverage Δ
...bosch/detekt/generator/collection/Configuration.kt 100.00% <ø> (ø)
...ekt/generator/collection/ConfigurationCollector.kt 62.38% <41.93%> (-7.62%) ⬇️
...rbosch/detekt/generator/collection/DefaultValue.kt 88.23% <83.33%> (-2.68%) ⬇️
.../io/gitlab/arturbosch/detekt/api/ConfigProperty.kt 96.77% <91.66%> (-3.23%) ⬇️
.../io/gitlab/arturbosch/detekt/generator/out/Yaml.kt 79.36% <95.65%> (+6.63%) ⬆️
...o/gitlab/arturbosch/detekt/api/ValuesWithReason.kt 100.00% <100.00%> (ø)
...tekt/generator/printer/RuleConfigurationPrinter.kt 100.00% <100.00%> (ø)
...ator/printer/defaultconfig/RuleSetConfigPrinter.kt 88.00% <100.00%> (-0.89%) ⬇️

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 a8cde4f...b27a563. Read the comment docs.

@marschwar marschwar marked this pull request as ready for review Mar 21, 2022
@cortinico
Copy link
Member

cortinico commented Mar 23, 2022

The idea is, that rule authors may decide that their rule supports values with reasons instead of values only. So instead of

Gotcha 👍 Looks great then.

@marschwar
Copy link
Contributor Author

marschwar commented Apr 3, 2022

Could we add this feature with 1.21?

@marschwar marschwar added this to the 1.21.0 milestone Apr 25, 2022
Copy link
Collaborator

@BraisGabin BraisGabin left a comment

Sorry for the late review! I missed the notification about this.

@BraisGabin
Copy link
Collaborator

BraisGabin commented May 22, 2022

@marschwar I think that we can merge this. Could you fix the conflicts?

Markus Schwarz added 2 commits May 22, 2022
…-reasons

# Conflicts:
#	detekt-generator/src/test/kotlin/io/gitlab/arturbosch/detekt/generator/collection/ConfigurationSpec.kt
#	detekt-generator/src/test/kotlin/io/gitlab/arturbosch/detekt/generator/collection/RuleCollectorSpec.kt
@marschwar
Copy link
Contributor Author

marschwar commented May 24, 2022

I have yet to figure out how to format the RuleCollectorSpec

@cortinico cortinico added the notable changes Marker for notable changes in the changelog label May 29, 2022
@marschwar
Copy link
Contributor Author

marschwar commented May 29, 2022

Ready to be merged from my point of view unless you would like me to add more tests?

Copy link
Member

@schalkms schalkms left a comment

Good to go
Thank you!

@marschwar marschwar merged commit 16b6658 into main Jun 1, 2022
19 of 20 checks passed
@marschwar marschwar deleted the feature/values-with-reasons branch Jun 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api core notable changes Marker for notable changes in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants