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

NestedScopeFunctions - Add rule for nested scope functions #4788

Merged
merged 7 commits into from
Jun 2, 2022

Conversation

WildOrangutan
Copy link
Contributor

@WildOrangutan WildOrangutan commented Apr 28, 2022

Fixes #4780

*
* [Reference](https://kotlinlang.org/docs/scope-functions.html)
*/
class NestedScopeFunctions(config: Config = Config.empty) : Rule(config) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: I'm not a super big fan of this rule explicitly mentioning ScopeFunctions while in reality you can customize the functions. So theoretically you could use this rule to check the nesting of your own non-scope functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, dunno. Judging by this, scope is general term and not explicitly tied to "scope functions" like apply.

I'm not a Kotlin expert tough, so let me know, if you have better naming in mind. Not sure how else to name it tough. One alternative I see is NestedFunctions, but that can be confusing (function definition in a function).

Copy link
Member

Choose a reason for hiding this comment

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

Happy to hear what @BraisGabin and others are thinking about this

Copy link
Member

Choose a reason for hiding this comment

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

I don't have a better name in mind right now. I'm fine with the current one.

@codecov
Copy link

codecov bot commented Apr 29, 2022

Codecov Report

Merging #4788 (1acbf18) into main (22bc63d) will increase coverage by 0.02%.
The diff coverage is 90.19%.

@@             Coverage Diff              @@
##               main    #4788      +/-   ##
============================================
+ Coverage     84.70%   84.73%   +0.02%     
- Complexity     3426     3432       +6     
============================================
  Files           491      492       +1     
  Lines         11253    11304      +51     
  Branches       2069     2077       +8     
============================================
+ Hits           9532     9578      +46     
- Misses          676      677       +1     
- Partials       1045     1049       +4     
Impacted Files Coverage Δ
...ch/detekt/rules/complexity/NestedScopeFunctions.kt 90.00% <90.00%> (ø)
...osch/detekt/rules/complexity/ComplexityProvider.kt 100.00% <100.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 22bc63d...1acbf18. Read the comment docs.

Copy link
Member

@BraisGabin BraisGabin left a comment

Choose a reason for hiding this comment

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

First of all: Thanks for this new PR! It looks really good.

I'm wondering, should we use here the same function matchers as we use in ForbiddenMethodCall?

    private val methods: List<FunctionMatcher> by config(
        listOf(
            "kotlin.io.print",
            "kotlin.io.println",
        )
    ) { it.map(FunctionMatcher::fromFunctionSignature) }

So instead of:

    functions:
      - 'apply'
      - 'run'
      - 'with'

We write:

    functions:
      - 'kotlin.apply'
      - 'kotlin.run'
      - 'kotlin.with'

This would avoid collisions between functions with the same name.

functions:
- 'apply'
- 'run'
- 'with'
Copy link
Member

Choose a reason for hiding this comment

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

I would add also and let too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, not sure. I think they are less problematic, since you can rename it. I think they maybe better fit to NestedBlockDepth or CognitiveComplexity?

Copy link
Member

Choose a reason for hiding this comment

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

For sure this is debatible. The good part is that this is configuratble so anyone could decide if they want to add or to remove some functions.

My 2 cents: If we add let and also and someone doesn't agree it's easy to remove. If we don't add them and someone want those too added is less likely that he/she add them. For that reason I'm thinking about adding them. But I'm ok about keeping them out. If someone else could share his/her opinion I think it would be great.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe smth in the middle - report nesting, when multiple it (or same name) are used, since that's problematic from my pov. Maybe in a next MR, so we don't drag this one too much. I can add them for time being.

@WildOrangutan
Copy link
Contributor Author

WildOrangutan commented Apr 30, 2022

I'm wondering, should we use here the same function matchers as we use in ForbiddenMethodCall?

Yeah, makes sense. Will add.

@schalkms
Copy link
Member

schalkms commented Jun 1, 2022

@cortinico do you agree with merging this PR? Do you have further comments for your approval?

Otherwise, I'd say LGTM.

@cortinico cortinico changed the title Add rule for nested scope functions NestedScopeFunctions - Add rule for nested scope functions Jun 2, 2022
Copy link
Member

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

Let's merge it 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rule for method nesting
4 participants