OutdatedDocumentation - Check if only public property is documented #6057
Conversation
Codecov Report
@@ Coverage Diff @@
## main #6057 +/- ##
=========================================
Coverage 84.85% 84.85%
- Complexity 3949 3979 +30
=========================================
Files 561 564 +3
Lines 13256 13312 +56
Branches 2315 2330 +15
=========================================
+ Hits 11248 11296 +48
- Misses 868 870 +2
- Partials 1140 1146 +6
|
| ) { | ||
| val doc = element.docComment ?: return | ||
| val docDeclarations = getDocDeclarations(doc) | ||
| if (docDeclarations.isNotEmpty()) { | ||
| val elementDeclarations = elementDeclarationsProvider() | ||
| if (!declarationsMatch(docDeclarations, elementDeclarations)) { | ||
| val isInternalPrimaryConstructor = element is KtClass && isInternalOrPrivate(element.primaryConstructor) |
There was a problem hiding this comment.
We call this method reportIfDocumentationIsOutdated with 3 different element types. However, in this method we check whether element has 1 specific type (e.g. KtClass). For me, this feels like a polymorphism code smell. See here for more details.
There was a problem hiding this comment.
Do we want me to move this check to visitClass method?
There was a problem hiding this comment.
Yes, just type KtClass of the 3 cases needs all those added checks.
There was a problem hiding this comment.
I have updated the code
| private fun declarationsMatch( | ||
| doc: List<Declaration>, | ||
| element: List<Declaration>, | ||
| shouldAllowOnlyPropertyDoc: Boolean, |
There was a problem hiding this comment.
Using boolean parameters is at least controversial. One obvious reason is that it makes code harder to read and maintain.
| /** | ||
| * Exception thrown from MyLibrary that can not be created outside of the library. | ||
| * @property errorCode Description of param | ||
| */ | ||
| class MyLibraryException internal constructor( | ||
| val errorCode: String, | ||
| cause: Throwable?, | ||
| ) : Exception(cause) |
There was a problem hiding this comment.
I think smaller test code snippets are sufficient here to understand what's being tested here.
There was a problem hiding this comment.
Is this somehow hampering the readability of the test code? I saw the other TCs which use MyClass but current TC is inspired by the issue #5217 also conveys why someone wants to document property in this manner
There was a problem hiding this comment.
Isn't shorter, more concise and less noisy to add code snippets like this?
/**
* Doc
* @property b desc
*/
class A internal constructor(val b: String)There was a problem hiding this comment.
TBH even the current TC was readable to me. But I have updated the cases. Thanks for reviewing it
cortinico
changed the title
Fixes #5217