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

UnusedPrivateMember should not report external classes/interfaces #4347

Merged
merged 2 commits into from Dec 1, 2021

Conversation

cortinico
Copy link
Member

Fixes #4340

@codecov
Copy link

codecov bot commented Nov 30, 2021

Codecov Report

Merging #4347 (9c30366) into main (6f41282) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 9c30366 differs from pull request most recent head e34f95c. Consider uploading reports for the commit e34f95c to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##               main    #4347   +/-   ##
=========================================
  Coverage     84.24%   84.24%           
  Complexity     3261     3261           
=========================================
  Files           474      474           
  Lines         10325    10326    +1     
  Branches       1826     1827    +1     
=========================================
+ Hits           8698     8699    +1     
  Misses          666      666           
  Partials        961      961           
Impacted Files Coverage Δ
...turbosch/detekt/rules/style/UnusedPrivateMember.kt 92.68% <100.00%> (+0.04%) ⬆️

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 6f41282...e34f95c. Read the comment docs.

@cortinico cortinico added this to the 1.20.0 milestone Dec 1, 2021
@cortinico cortinico added the rules label Dec 1, 2021
@BraisGabin
Copy link
Member

🤔 I don't get why things that aren't private are flagged by this rule. I'm wondering if there is an underlying bug here. I mean, why this rule mixes external and private?

@cortinico
Copy link
Member Author

🤔 I don't get why things that aren't private are flagged by this rule. I'm wondering if there is an underlying bug here. I mean, why this rule mixes external and private?

Yup the tests were confusing, I've fixed them. The rule is reporting private members and parameters that are unused and in my tests I accidentally removed the parameters.

@BraisGabin
Copy link
Member

I should be missing something here... What isprivate in this snipped?

                external class Bugsnag {
                    companion object {
                        fun start(value: Int)
                        fun notify(error: String)
                    }
                }

This rule should only flag code that is marked as private right? Why was this code triggering the rule?

@cortinico
Copy link
Member Author

I should be missing something here... What isprivate in this snipped?

The confusion is that UnusedPrivateMember is doing more than what its name says:

* Reports unused private properties, function parameters and functions.
* If these private elements are unused they should be removed. Otherwise, this dead code
* can lead to confusion and potential bugs.

Ideally it should be split in UnusedFunction, UnusedParameter and UnusedPrivateMember

@BraisGabin
Copy link
Member

I get it now! Thanks for the explanation. That have sense.

@BraisGabin BraisGabin merged commit 9bfba9a into detekt:main Dec 1, 2021
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.

UnusedPrivateMember should ignore external classes
2 participants