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

Report KDoc comments that refer to non-public properties of a class #4768

Conversation

VitalyVPinchuk
Copy link
Contributor

@VitalyVPinchuk VitalyVPinchuk commented Apr 25, 2022

New rule to address the issue (#4462) of breaking encapsulation principle when KDoc comments have references to non-public properties.
Clients do not need to know the implementation details.

Screenshot
Screenshot 2022-04-26 at 22 07 12

New rule to address the issue of breaking encapsulation principle when KDoc comments have references to encapsulated (private) properties.
Clients do not need to know the implementation details.
@vercel
Copy link

vercel bot commented Apr 25, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
detekt ❌ Failed (Inspect) Apr 26, 2022 at 8:46PM (UTC)

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.

Thanks for adding this 👍

@cortinico cortinico added this to the 1.21.0 milestone Apr 25, 2022
@vercel
Copy link

vercel bot commented Apr 26, 2022

@VitalyVPinchuk is attempting to deploy a commit to the Detekt Team on Vercel.

A member of the Team first needs to authorize it.

@VitalyVPinchuk VitalyVPinchuk changed the title Report KDoc comments that refer to encapsulated properties of a class Report KDoc comments that refer to non-public properties of a class Apr 26, 2022
@VitalyVPinchuk VitalyVPinchuk force-pushed the KDoc-should-not-refer-to-encapsulated-properties branch from ba2c656 to 8e1bccc Compare April 26, 2022 19:46
@VitalyVPinchuk VitalyVPinchuk force-pushed the KDoc-should-not-refer-to-encapsulated-properties branch from 8e1bccc to b8700b6 Compare April 26, 2022 19:51
@marschwar
Copy link
Contributor

Just curious - Should this rule also check the KDoc of methods and properties to verify it is not leaking private information?

class C {
    private var a: Int = 0

    /**
     * Mentioning [a]
     */
    val b = "string"

    /**
     * Mentioning [a]
     */
    fun foo() {
        print("foo $a")
    }
}

@VitalyVPinchuk
Copy link
Contributor Author

Just curious - Should this rule also check the KDoc of methods and properties to verify it is not leaking private information?

I think it's a good idea!
Should I add these checks?

@marschwar
Copy link
Contributor

Should I add these checks?

If you think they are valid, you could do so. I am just afraid that the rule will become overly complicated and hard to maintain.

To be absolutely honest, I am not the biggest fan of documentation rules to begin with. Since you brought up the issue, you seem to have a clear use case.

@codecov
Copy link

codecov bot commented Apr 28, 2022

Codecov Report

Merging #4768 (4679ae2) into main (7960453) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #4768      +/-   ##
============================================
+ Coverage     84.69%   84.76%   +0.06%     
- Complexity     3418     3447      +29     
============================================
  Files           490      492       +2     
  Lines         11235    11302      +67     
  Branches       2068     2080      +12     
============================================
+ Hits           9516     9580      +64     
+ Misses          675      673       -2     
- Partials       1044     1049       +5     
Impacted Files Coverage Δ
...etekt/generator/printer/defaultconfig/Exclusion.kt 96.77% <100.00%> (+0.10%) ⬆️
...detekt/rules/documentation/CommentSmellProvider.kt 100.00% <100.00%> (ø)
...s/documentation/KDocReferencesNonPublicProperty.kt 100.00% <100.00%> (ø)
...coroutines/SuspendFunWithCoroutineScopeReceiver.kt 80.00% <0.00%> (-3.34%) ⬇️
...gitlab/arturbosch/detekt/rules/style/UseOrEmpty.kt 74.07% <0.00%> (-3.01%) ⬇️
...lab/arturbosch/detekt/rules/style/VarCouldBeVal.kt 80.39% <0.00%> (-0.03%) ⬇️
...arturbosch/detekt/generator/printer/RulePrinter.kt 100.00% <0.00%> (ø)
...rturbosch/detekt/rules/style/StyleGuideProvider.kt 100.00% <0.00%> (ø)
...urbosch/detekt/rules/style/UnnecessaryBackticks.kt 85.00% <0.00%> (ø)
...n/io/github/detekt/report/html/HtmlOutputReport.kt 95.65% <0.00%> (+0.14%) ⬆️
... and 4 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 7960453...4679ae2. Read the comment docs.

@VitalyVPinchuk VitalyVPinchuk force-pushed the KDoc-should-not-refer-to-encapsulated-properties branch 2 times, most recently from b14a757 to e99cd26 Compare May 5, 2022 04:46
@VitalyVPinchuk VitalyVPinchuk force-pushed the KDoc-should-not-refer-to-encapsulated-properties branch from e99cd26 to eed733c Compare May 5, 2022 05:07
@cortinico
Copy link
Member

If you think they are valid, you could do so. I am just afraid that the rule will become overly complicated and hard to maintain.

Agree here. I would not add those extra checks unless we have a real use case or a user need for them

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.

Code looks good to me. Just left minor comments to address

@BraisGabin BraisGabin merged commit 4dd9ac4 into detekt:main May 22, 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.

None yet

4 participants