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

ReturnCount.excludedFunctions should be a List<String> #5081

Merged
merged 1 commit into from Jul 23, 2022

Conversation

gouri-panda
Copy link
Contributor

closes: #5063

@codecov
Copy link

codecov bot commented Jul 15, 2022

Codecov Report

Merging #5081 (caa95a9) into main (6bce28f) will increase coverage by 0.05%.
The diff coverage is 85.71%.

Current head caa95a9 differs from pull request most recent head 1dc5b15. Consider uploading reports for the commit 1dc5b15 to get more accurate results

@@             Coverage Diff              @@
##               main    #5081      +/-   ##
============================================
+ Coverage     84.87%   84.92%   +0.05%     
- Complexity     3575     3605      +30     
============================================
  Files           500      502       +2     
  Lines         11785    11876      +91     
  Branches       2197     2227      +30     
============================================
+ Hits          10002    10086      +84     
+ Misses          692      691       -1     
- Partials       1091     1099       +8     
Impacted Files Coverage Δ
...in/io/gitlab/arturbosch/detekt/api/SplitPattern.kt 92.59% <50.00%> (ø)
...itlab/arturbosch/detekt/rules/style/ReturnCount.kt 93.61% <100.00%> (+0.43%) ⬆️
...io/gitlab/arturbosch/detekt/core/BindingContext.kt 40.00% <0.00%> (-1.18%) ⬇️
...lab/arturbosch/detekt/formatting/FormattingRule.kt 97.87% <0.00%> (-0.09%) ⬇️
.../arturbosch/detekt/formatting/wrappers/Wrapping.kt 100.00% <0.00%> (ø)
...turbosch/detekt/formatting/wrappers/PackageName.kt 100.00% <0.00%> (ø)
...rturbosch/detekt/rules/style/StyleGuideProvider.kt 100.00% <0.00%> (ø)
...osch/detekt/formatting/wrappers/MultiLineIfElse.kt 100.00% <0.00%> (ø)
...ch/detekt/formatting/wrappers/AnnotationSpacing.kt 100.00% <0.00%> (ø)
...ch/detekt/formatting/wrappers/EnumEntryNameCase.kt 100.00% <0.00%> (ø)
... and 18 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 6bce28f...1dc5b15. Read the comment docs.

@cortinico
Copy link
Member

verify-generated-config-file is broken. Could you re-run ./gradlew generateDocumentation?

@Configuration("define a free-form comma separated list of function names to be ignored by this check")
private val excludedFunctions: SplitPattern by config("equals") { SplitPattern(it) }
@Configuration("define a list of function names to be ignored by this check")
private val excludedFunctions: List<String> by config(emptyList())
Copy link
Collaborator

Choose a reason for hiding this comment

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

You are introducing a breaking change here. You should keep "equals".

And I don't know why the tests doesn't complain but looking at the code of SplitPattern this code doesn't do the same as before, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are introducing a breaking change here. You should keep `"equals"

Ok

And I don't know why the tests doesn't complain but looking at the code of SplitPattern this code doesn't do the same as before, right?

Yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@BraisGabin Should we change the SplitPattern implementation and leave the signature of ReturnCount as before i.e private val excludedFunctions: SplitPattern by config("equals") { SplitPattern(it) }?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this the right way to change?
ReturnCount.kt

    private val excludedFunctions: SplitPattern by config(listOf("equals")) { SplitPattern(it.toString()) }

SplitPattern.kt

@Suppress("detekt.SpreadOperator")
    private val excludes = text
        .commaSeparatedPattern(*delimiters.toCharArray().map { it.toString() }.toTypedArray())
        .mapIf(removeTrailingAsterisks) { seq ->
            seq.map { it.removePrefix("*") }
                .map { it.removeSuffix("*") }
                .map {it.removePrefix("[")}
                .map { it.removeSuffix("]") }
        }
        .toList()

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would look at what SplitPattern does and reproduce it in this rule. I'm sure that I did this time on other classes some time ago but for some reason I missed this one.

@@ -1,14 +1,8 @@
@file:Suppress("WildcardImport", "NoWildcardImports")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't use wildcard imports.

@Configuration("define a free-form comma separated list of function names to be ignored by this check")
private val excludedFunctions: SplitPattern by config("equals") { SplitPattern(it) }
@Configuration("define a list of function names to be ignored by this check")
private val excludedFunctions: SplitPattern by config(listOf("equals")) { SplitPattern(it.toString()) }
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you want to go this way you can use it.joinString(",") (or something similar, I can't recall the signature). This way you don't need to change SplitPattern. That could have unexpected implications.

fun matches(value: String): List<String> =
excludes.filter { value.contains(it, ignoreCase = true) }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Reduce the changed lines

Suggested change
fun matches(value: String): List<String> =
excludes.filter { value.contains(it, ignoreCase = true) }
fun matches(value: String): List<String> = excludes.filter { value.contains(it, ignoreCase = true) }

fun contains(value: String?): Boolean =
excludes.any { value?.contains(it, ignoreCase = true) == true }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Reduce the changed lines

Suggested change
fun contains(value: String?): Boolean =
excludes.any { value?.contains(it, ignoreCase = true) == true }
fun contains(value: String?): Boolean = excludes.any { value?.contains(it, ignoreCase = true) == true }

SplitPattern(
it.joinToString(
","
)
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
SplitPattern(
it.joinToString(
","
)
)
SplitPattern(it.joinToString(","))

private fun shouldBeIgnored(function: KtNamedFunction) =
excludedFunctions.contains(function.name)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Reduce the changed lines

Suggested change
private fun shouldBeIgnored(function: KtNamedFunction) =
excludedFunctions.contains(function.name)
private fun shouldBeIgnored(function: KtNamedFunction) = excludedFunctions.contains(function.name)

@BraisGabin BraisGabin added this to the 1.22.0 milestone Jul 18, 2022
@BraisGabin BraisGabin merged commit 13d823c into detekt:main Jul 23, 2022
19 checks passed
VitalyVPinchuk pushed a commit to VitalyVPinchuk/detekt that referenced this pull request Jul 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ReturnCount.excludedFunctions should be a List<String> instead of a String
3 participants