detekt / detekt

Static code analysis for Kotlin
https://detekt.dev
Apache License 2.0
6.1k stars 758 forks source link

Add class name exclusions to UndocumentedPublicClass rule #7252

Closed mgulbronson closed 4 weeks ago

mgulbronson commented 1 month ago

This PR adds a parameter to the UndocumentedPublicClass rule to exclude exact class or object names. The rationale for this rule is situations like the following:

class Foo private constructor (val s: String) {
  class Factory {
    fun create(s: String): Foo = Foo(s)
  }
}

The outer class Foo should be documented to explain what it does and why. But the inner class Foo.Factory is very much self-documenting: it is a factory that creates new Foos. The same idea can apply to a Builder class.

Currently the only way to exclude these kinds of classes is to manually @Suppress them or disable the rule for all nested classes with the searchInNestedClass config. But doing so would disable it for lots of cases where the nested class should be documented because it's more complex than a factory class with a single create function.


I made this change with the nested Factory class example above in mind, but I implemented it to look at all classes. I think it would primarily apply to nested classes where the outer class provides all the needed documentation, but I wasn't sure if it made sense to limit the usage in case there was a use case for non-nested classes that I couldn't think of. I'm open to changing this to only apply to nested classes if people think that makes more sense.

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 84.71%. Comparing base (fe00dd9) to head (d498164).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #7252 +/- ## ========================================= Coverage 84.71% 84.71% - Complexity 3993 3994 +1 ========================================= Files 578 578 Lines 12163 12165 +2 Branches 2494 2495 +1 ========================================= + Hits 10304 10306 +2 Misses 626 626 Partials 1233 1233 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

cortinico commented 1 month ago

But the inner class Foo.Factory is very much self-documenting: it is a factory that creates new Foos

This feels too specific to your example, as it's extremely simple. I'm trying to understand if your argument is: "We don't need to document this because is called Factory".

I would argue that you could have really complex factories which you might want to document + adding KDoc for those classes as well makes for better IDE integration

mgulbronson commented 1 month ago

I'm curious. Do you have any references for this style of documenting?

The outer class Foo should be documented to explain what it does and why. But the inner class Foo.Factory is very much self-documenting: it is a factory that creates new Foos. The same idea can apply to a Builder class.

@schalkms No references, but I feel it's fairly self-evident that nested classes are at least partially documented by their outer class — the whole reason they're nested is that they're closely intertwined and share some context, right?

Considering this rule already has an option to not search in nested classes I think it's fair to say that the rule authors are already aware of this nested/outer class documentation pattern.

mgulbronson commented 1 month ago

But the inner class Foo.Factory is very much self-documenting: it is a factory that creates new Foos

This feels too specific to your example, as it's extremely simple. I'm trying to understand if your argument is: "We don't need to document this because is called Factory".

I would argue that you could have really complex factories which you might want to document + adding KDoc for those classes as well makes for better IDE integration

@cortinico it's a very fair point — my example is definitely a bit contrived (and a lot simple).

I'm trying to understand if your argument is: "We don't need to document this because is called Factory".

Mostly yes. But you're right that just because a class is called Factory doesn't necessarily mean it's not complex. I think it also comes down to the particular codebase's style and how these kinds of classes are used. I'm coming at this from the perspective of someone who was a bit annoyed at having to document a very trivial Factory class, not as someone who was grateful that a complex factory class was well-documented 😆. So I recognize that this proposal might be a bit too specific to my use case.

BraisGabin commented 1 month ago

I have a similar use case for a similar rule. In my case I don't want to document the parameter Modifier for my @Composable functions. In the context on compose everyone knows what is the modifier and documenting it hurts more than helps. So I guess that it's OK to allow to ignore some classes by name too.

mgulbronson commented 1 month ago

@cortinico @BraisGabin let me know if there's anything you'd like to see changed to get this merged! I understand my usages example is a bit contrived so if you've got suggestions on how to make this change work better considering more complex cases I'm all ears

cortinico commented 1 month ago

So @BraisGabin was also a valid point. I'm not opposing this rule, I'd just like to collect more evidence like the one provided by Brais before we actually implement this

mgulbronson commented 4 weeks ago

I think I'll close this for now. if @BraisGabin goes forward with the change he discussed in the UndocumentedPublicFunction rule then I may re-open this one following their style. Thanks for your consideration folks!