Skip to content

UndocumentedPublicProperty and UndocumentedPublicFunction should include objects #3940

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 6 commits into from
Jul 7, 2021
Merged

UndocumentedPublicProperty and UndocumentedPublicFunction should include objects #3940

merged 6 commits into from
Jul 7, 2021

Conversation

anthonycr
Copy link
Contributor

@anthonycr anthonycr commented Jul 6, 2021

Summary

Given an object with an undocumented public function or property, I would expect the UndocumentedPublicFunction rule or UndocumentedPublicProperty rule to fail the build. However, the behavior of the rules is inconsistent in regards to objects.

The following code is failed when the rules are enabled and an object is nested in a class, as expected:

class Test {
  object InnerObject {
    // Fails, expected
    fun noComment1() {}
    // Fails, expected
    val a = 1
  }
}

However, if the object is promoted to a top level object, then the rules will not fail the function or property.

object Test {
  // Passes, unexpected
  fun noComment1() {}
  // Passes, unexpected
  val a = 1
}

Changes

I discovered that the containingClass() function used in both rules to determine if the function or property is part of a public class returns null if the function or property is in an object. We can change this behavior to check if the object is public or not by replacing the usage of containingClass() with containingClassOrObject. In this way, we can treat an undocumented function or property in an object the same as one in a regular class.

I changed both UndocumentedPublicFunction and UndocumentedPublicProperty to utilize containingClassOrObject instead of containingClass(). I also added test cases for public and private object cases as well as a nested object test case. For the test cases I added to the UndocumentedPublicPropertySpec, I renamed two of the existing test cases with ambiguous names to avoid confusion with the test cases I added.

anthonycr added 4 commits July 6, 2021 11:18
The class returned was null for functions inside an object, resulting in a false negative, so we instead use containingClassOrObject to determine if the containing object is public or not.
…etermining eligibility for UndocumentedPublicProperty check

containingClass() returns null when checking an object not nested in another class, making it ineligible to check. Switching to containingClassOrObject allows properties in an obect to be checked.
@cortinico cortinico added the rules label Jul 6, 2021
@cortinico cortinico added this to the 1.18.0 milestone Jul 6, 2021
@cortinico
Copy link
Member

Thanks for the fix 🙏 That's really appreciated.

@codecov
Copy link

codecov bot commented Jul 6, 2021

Codecov Report

Merging #3940 (9018979) into main (5048b8b) will not change coverage.
The diff coverage is 0.00%.

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

@@            Coverage Diff            @@
##               main    #3940   +/-   ##
=========================================
  Coverage     83.44%   83.44%           
  Complexity     3149     3149           
=========================================
  Files           456      456           
  Lines          9014     9014           
  Branches       1754     1754           
=========================================
  Hits           7522     7522           
  Misses          565      565           
  Partials        927      927           
Impacted Files Coverage Δ
.../rules/documentation/UndocumentedPublicFunction.kt 85.71% <0.00%> (ø)
.../rules/documentation/UndocumentedPublicProperty.kt 88.46% <0.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 5048b8b...89ad157. Read the comment docs.

Copy link
Member

@schalkms schalkms left a comment

Choose a reason for hiding this comment

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

Does the rule also flag interfaces with this implementation?

@anthonycr
Copy link
Contributor Author

@schalkms yes, interfaces were being flagged before with containingClass() and are being flagged with this updated implementation, as the interface is returned as the encapulating "class" by the containingClassOrObject property. I just added a test case for interfaces to both the property and function test specs as it doesn't look like that case was explicitly being tested for yet.

@schalkms schalkms merged commit b2aba98 into detekt:main Jul 7, 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.

3 participants