Add support for fallback property by marschwar · Pull Request #3675 · detekt/detekt

marschwar · 2021-04-11T18:00:31Z

This PR relates to #3670 and adds support for annotated properties that have been renamed. Example:

without annotation

    private val functionThreshold: Int =
        valueOrDefault(FUNCTION_THRESHOLD, valueOrDefault(THRESHOLD, DEFAULT_FUNCTION_THRESHOLD))

with annotation

    private val functionThreshold: Int  by fallbackConfig(
        fallbackProperty = "threshold", 
        defaultValue = DEFAULT_FUNCTION_THRESHOLD
    )

codecov · 2021-04-11T18:17:09Z

Codecov Report

Merging #3675 (d3395c3) into main (50b453b) will increase coverage by 0.01%.
The diff coverage is 74.41%.

❗ Current head d3395c3 differs from pull request most recent head d6fef86. Consider uploading reports for the commit d6fef86 to get more accurate results

@@             Coverage Diff              @@
##               main    #3675      +/-   ##
============================================
+ Coverage     78.04%   78.05%   +0.01%     
- Complexity     2883     2891       +8     
============================================
  Files           473      473              
  Lines          9300     9329      +29     
  Branches       1767     1785      +18     
============================================
+ Hits           7258     7282      +24     
+ Misses         1078     1076       -2     
- Partials        964      971       +7

Impacted Files	Coverage Δ	Complexity Δ
...ekt/generator/collection/ConfigurationCollector.kt	`73.03% <70.37%> (+2.44%)`	`28.00 <11.00> (+8.00)`
...b/arturbosch/detekt/api/internal/ConfigProperty.kt	`83.33% <81.25%> (+3.33%)`	`0.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 50b453b...d6fef86. Read the comment docs.

picklebento · 2021-04-13T00:58:54Z

+fun <T : Any> config(defaultValue: T): ReadOnlyProperty<ConfigAware, T> =
    SimpleConfigProperty(defaultValue)

+fun <T : Any> fallbackConfig(fallbackProperty: String, defaultValue: T): ReadOnlyProperty<ConfigAware, T> =


A wild idea: Could we use ReadOnlyProperty<ConfigAware, T> as the type of fallbackProperty? This can help us guarantee that there is no typo to be made as String.

I played around with this idea, but I don't see how that would be possible. The concern of having a typo is very valid IMO so I added a check in the config collector.

cortinico · 2021-05-02T14:03:53Z

    private val functionThreshold: Int  by fallbackConfig(
        fallbackProperty = "threshold", 
        defaultValue = DEFAULT_FUNCTION_THRESHOLD
    )

I came a bit late to the party but I have a couple of (maybe dumb) questions here:

Could we implement this as an property for config with default value (either null or ""):

fun config(fallback: String = "", defaultValue: String): ReadOnlyProperty<ConfigAware, String> = simpleConfig(fallback, defaultValue)

If 1. is not an option, could we call the delegate function configWithFallback? I think it's going to be more discoverable (especially for autocompletion and similar) and it brings more context on what the delegate is actually doing.

marschwar · 2021-05-02T14:18:58Z

I like the suggestion from @chao2zhang to give the each config delegate its own name so we don't just have the same function overloaded multiple times.

When it comes to naming I am open for anything. I understand the reasoning and like your suggestion. What would you call the delegate that expects a List to be configured?

config            -> config
fallbackConfig    -> configWithFallback
listConfig        -> ??? configOfList ???

cortinico · 2021-05-02T14:20:39Z

What would you call the delegate that expects a List to be configured?

How about configList?

picklebento · 2021-05-02T18:42:53Z

+        return checkNotNull(defaultArgument.getArgumentExpression())
+    }
+
+    private object ConfigWithFallbackSupport {


Today I learned: We can define a non-companion object inside a class.

BraisGabin · 2021-05-03T06:59:20Z

Why do we need configList? Isn't enought to use config? And, if we need configWithFallback don't we need configListWithFallback too? I'd vote for an unified delegate config.

marschwar · 2021-05-03T10:36:09Z

I agree that the differenciation between config and configList is a bit awkward. Let me see if there is something that can be done.

…erty

marschwar · 2021-05-03T14:06:48Z

If this is something we can agree on, I would clean up and add more tests.

BraisGabin · 2021-05-03T15:06:06Z

+        is Boolean,
+        is Int,
+        is Long -> configAware.valueOrDefault(propertyName, defaultValue)
+        else -> error("${defaultValue.javaClass} is not supported as ")


Maybe using the code from here: #3676 (comment) we could make this more type safe.

The problem here is, that fun <T : Any> config(defaultValue: T): ReadOnlyProperty<ConfigAware, T> removes most of the type safety since a rule author could write val prop: Regex by config(Regex("aaa")) which would compile but fail at runtime. With the code from this PR it is more explicit and we can provide a specific error message if the provided type is not supported.

If we want that type of security (I agree with it). We could add a config file for each type. This way we ensure even more the type safety.

BraisGabin · 2021-05-03T15:16:13Z

If this is something we can agree on, I would clean up and add more tests.

I think that the API looks nice :)

BraisGabin · 2021-05-05T07:07:14Z

This looks fine now. We can iterate later if we want.

marschwar · 2021-05-07T19:04:08Z

Is there anything I can do to help getting this merged?

* Add support for fallback property * Introduce listConfig and fallbackConfig * Verify that the configured fallback property exists * replace config with listConfig where appropriate * rename fallbackConfig -> configWithFallback * rename parameter fallbackProperty -> fallbackPropertyName * Integrate configList into SimpleConfigProperty and FallbackConfigProperty * Improve error handling and add tests * Inline values to pass custom checks Co-authored-by: Markus Schwarz <post@markus-schwarz.net>

marschwar force-pushed the fallback-config-property branch from 6255a2a to 7526a32 Compare April 11, 2021 18:14

marschwar mentioned this pull request Apr 11, 2021

Add support for transformer function in config property delegate #3676

Merged

picklebento approved these changes Apr 11, 2021

View reviewed changes

Comment thread detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/internal/ConfigProperty.kt Outdated

picklebento reviewed Apr 13, 2021

View reviewed changes

Markus Schwarz added 3 commits May 1, 2021 15:05

Add support for fallback property

ee6e368

Introduce listConfig and fallbackConfig

f491778

Verify that the configured fallback property exists

2cc6c91

marschwar force-pushed the fallback-config-property branch from 233fc73 to 7eee539 Compare May 1, 2021 13:21

replace config with listConfig where appropriate

385cf59

marschwar force-pushed the fallback-config-property branch from 7eee539 to 385cf59 Compare May 1, 2021 16:37

marschwar mentioned this pull request May 2, 2021

Use @Configuration annotation instead of kdoc in all rules #3670

Closed

16 tasks

rename fallbackConfig -> configWithFallback

9724a5e

picklebento approved these changes May 2, 2021

View reviewed changes

rename parameter fallbackProperty -> fallbackPropertyName

8dfffdc

Integrate configList into SimpleConfigProperty and FallbackConfigProp…

5feffd0

…erty

BraisGabin reviewed May 3, 2021

View reviewed changes

Markus Schwarz added 2 commits May 3, 2021 19:07

Improve error handling and add tests

1bf3037

Inline values to pass custom checks

d6fef86

BraisGabin approved these changes May 5, 2021

View reviewed changes

BraisGabin merged commit 7cc0c9d into detekt:main May 7, 2021

marschwar deleted the fallback-config-property branch May 8, 2021 16:19

cortinico added this to the 1.17.0 milestone May 12, 2021

Uh oh!

Conversation

marschwar commented Apr 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Apr 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

picklebento Apr 13, 2021

Choose a reason for hiding this comment

Uh oh!

marschwar May 1, 2021

Choose a reason for hiding this comment

Uh oh!

cortinico commented May 2, 2021

Uh oh!

marschwar commented May 2, 2021

Uh oh!

cortinico commented May 2, 2021

Uh oh!

Uh oh!

picklebento May 2, 2021

Choose a reason for hiding this comment

Uh oh!

BraisGabin commented May 3, 2021

Uh oh!

marschwar commented May 3, 2021

Uh oh!

marschwar commented May 3, 2021

Uh oh!

BraisGabin May 3, 2021

Choose a reason for hiding this comment

Uh oh!

marschwar May 3, 2021

Choose a reason for hiding this comment

Uh oh!

BraisGabin May 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BraisGabin commented May 3, 2021

Uh oh!

BraisGabin commented May 5, 2021

Uh oh!

marschwar commented May 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

marschwar commented Apr 11, 2021 •

edited

Loading

codecov Bot commented Apr 11, 2021 •

edited

Loading

BraisGabin May 3, 2021 •

edited

Loading