Skip to content

Deprecate ignoreOverriddenFunction/s in favor of ignoreOverridden #2132

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

Merged
merged 11 commits into from
Jan 8, 2020
Merged

Deprecate ignoreOverriddenFunction/s in favor of ignoreOverridden #2132

merged 11 commits into from
Jan 8, 2020

Conversation

BraisGabin
Copy link
Member

@BraisGabin BraisGabin commented Nov 27, 2019

Closes #2096

EmptyFunctionBlock, FunctionParameterNaming and MemberNameEqualsClassName uses the configuration flag ignoreOverriddenFunction(s) but the rest of rules uses the generic ignoreOverridden. This PR change this to get an API a bit more consistent.

@codecov-io
Copy link

codecov-io commented Nov 29, 2019

Codecov Report

Merging #2132 into master will increase coverage by 0.13%.
The diff coverage is 72.91%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2132      +/-   ##
============================================
+ Coverage      81.2%   81.34%   +0.13%     
- Complexity     2064     2067       +3     
============================================
  Files           340      341       +1     
  Lines          5910     5965      +55     
  Branches       1076     1085       +9     
============================================
+ Hits           4799     4852      +53     
+ Misses          535      532       -3     
- Partials        576      581       +5
Impacted Files Coverage Δ Complexity Δ
...in/io/gitlab/arturbosch/detekt/api/Notification.kt 0% <0%> (ø) 0 <0> (?)
.../io/gitlab/arturbosch/detekt/generator/out/Yaml.kt 58.62% <0%> (-6.77%) 0 <0> (ø)
...gitlab/arturbosch/detekt/generator/out/Markdown.kt 20.68% <0%> (-0.74%) 4 <0> (ø)
...rturbosch/detekt/rules/empty/EmptyFunctionBlock.kt 92.3% <100%> (ø) 12 <0> (ø) ⬇️
...enerator/printer/rulesetpage/RuleSetPagePrinter.kt 86.84% <100%> (+1.54%) 17 <0> (+1) ⬆️
...arturbosch/detekt/core/ModificationNotification.kt 75% <100%> (+8.33%) 1 <0> (-1) ⬇️
...turbosch/detekt/api/internal/SimpleNotification.kt 75% <100%> (+25%) 3 <3> (+2) ⬆️
...h/detekt/rules/naming/MemberNameEqualsClassName.kt 88.46% <100%> (ø) 14 <0> (ø) ⬇️
...ekt/generator/printer/rulesetpage/ConfigPrinter.kt 76.74% <100%> (+1.74%) 8 <0> (ø) ⬇️
...bosch/detekt/generator/collection/Configuration.kt 100% <100%> (ø) 5 <2> (+1) ⬆️
... and 7 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 3d4ea24...10a3133. Read the comment docs.

@arturbosch
Copy link
Member

arturbosch commented Nov 29, 2019

Cool, looks good to me.
I like the deprecation approach for 1.3. We can remove them in 1.4.

I also like the approach to print deprecation messages in validateConfig:

val DEPRECATED_PROPERTIES = CommaSeparatedPattern(setOf(
    ".*>.*>ignoreOverriddenFunction",
    ".*>.*>ignoreOverriddenFunctions"
).joinToString(",")).mapToRegex()

@Suppress("UNCHECKED_CAST", "ComplexMethod")
fun validateConfig(
    config: Config,
    baseline: Config,
    excludePatterns: Set<Regex> = CommaSeparatedPattern(DEFAULT_PROPERTY_EXCLUDES).mapToRegex()
): List<Notification> {
    ....
    fun testKeys(current: Map<String, Any>, base: Map<String, Any>, parentPath: String?) {
        for (prop in current.keys) {

            val propertyPath = "${if (parentPath == null) "" else "$parentPath>"}$prop"

            if (excludePatterns.any { it.matches(propertyPath) }) {
                continue
            }

            if (DEPRECATED_PROPERTIES.isNotEmpty() && DEPRECATED_PROPERTIES.any { it.matches(propertyPath) }) {
                notifications.add(propertyIsDeprecated(propertyPath))
            }
           ....
}

@BraisGabin
Copy link
Member Author

I did the change, but if I add something to notifications the build run fails. So they are not warnings, they are errors.

Do I add a level to the Notification class?

In the other hand the notification is not really useful:

Property 'empty-blocks>EmptyFunctionBlock>ignoreOverriddenFunctions' is deprecated.
Property 'naming>FunctionParameterNaming>ignoreOverriddenFunctions' is deprecated.
Property 'naming>MemberNameEqualsClassName>ignoreOverriddenFunction' is deprecated.

It say that they're deprecated but the messages don't point out how to fix them.

@BraisGabin BraisGabin marked this pull request as ready for review December 28, 2019 20:06
@BraisGabin
Copy link
Member Author

Ok, I did some little changes to this PR and I added a new property level to the Notification class.

@arturbosch
Copy link
Member

I did the change, but if I add something to notifications the build run fails. So they are not warnings, they are errors.

Do I add a level to the Notification class?

In the other hand the notification is not really useful:

Property 'empty-blocks>EmptyFunctionBlock>ignoreOverriddenFunctions' is deprecated.
Property 'naming>FunctionParameterNaming>ignoreOverriddenFunctions' is deprecated.
Property 'naming>MemberNameEqualsClassName>ignoreOverriddenFunction' is deprecated.

It say that they're deprecated but the messages don't point out how to fix them.

Ah, I totally missed your message. Level sounds good to me. Lets introduce a third level info which is default and used by ModificationNotification and "Successfully generated ${report.name} at $filePath" and "detekt finished in $time ms.". Warning and Error are already perfect +1

@arturbosch arturbosch added this to the 1.4.0 milestone Dec 28, 2019
@BraisGabin
Copy link
Member Author

Done

@arturbosch arturbosch merged commit baefc1e into detekt:master Jan 8, 2020
@BraisGabin BraisGabin deleted the ignore-overridden branch May 20, 2021 20:26
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.

Deprecate ignoreOverriddenFunction(s) in favor fo ignoreOverridden
3 participants