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

#4169 OutdatedDocumentation rule #4185

Merged
merged 11 commits into from
Oct 19, 2021

Conversation

lukaszosowicki
Copy link
Contributor

Hi, I've prepared implementation of OutdatedDocumentation rule proposed in #4169

When KDoc is present and contains any @param or @property tag, rule will check if params and properties of class/function/constructor declaration matches the ones from KDoc.

By default, both type and value parameters are considered, but I've added configuration for turning off checks for type parameters. Also the order of declarations matters in default configuration, but can be turned off as well.

If you want to suggest additional test scenarios or configurable behaviour feel free to comment and I will be happy to enhance this PR.

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.

Great work 💪 Thanks for taking the time to write this. I've left some comments. Let me know if you need some help or are unclear.

Comment on lines 171 to 193
private data class MutableDeclarations(
val params: MutableList<String> = mutableListOf(),
val props: MutableList<String> = mutableListOf()
) {
fun toDeclarations(): Declarations {
return Declarations(
params = params,
props = props
)
}
}

private data class Declarations(
val params: List<String> = listOf(),
val props: List<String> = listOf()
)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need both? I believe we could rewrite the code to be fully immutable here. Specifically we could get rid of MutableDeclarations entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've refactored this part a bit to get rid of mutable version and use more functional style

@cortinico cortinico added this to the 1.19.0 milestone Oct 15, 2021
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.

Looks good on my end 💪 Great work and thanks for doing it!
Let's wait for another maintainers' review and we should be good to go

@codecov
Copy link

codecov bot commented Oct 18, 2021

Codecov Report

Merging #4185 (daa18b6) into main (50b9314) will increase coverage by 0.06%.
The diff coverage is 87.50%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #4185      +/-   ##
============================================
+ Coverage     84.12%   84.19%   +0.06%     
- Complexity     3205     3233      +28     
============================================
  Files           467      468       +1     
  Lines         10113    10185      +72     
  Branches       1774     1786      +12     
============================================
+ Hits           8508     8575      +67     
+ Misses          673      671       -2     
- Partials        932      939       +7     
Impacted Files Coverage Δ
...etekt/rules/documentation/OutdatedDocumentation.kt 87.32% <87.32%> (ø)
...detekt/rules/documentation/CommentSmellProvider.kt 100.00% <100.00%> (ø)
...itlab/arturbosch/detekt/api/internal/Signatures.kt 85.71% <0.00%> (+8.16%) ⬆️

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 50b9314...daa18b6. Read the comment docs.

Copy link
Member

@BraisGabin BraisGabin left a comment

Choose a reason for hiding this comment

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

Really good test coverage :)

* @param someParam Description of param
* @property someProp Description of property
*/
class MyClass(otherParam: String, val someProp: String)
Copy link
Member

Choose a reason for hiding this comment

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

What happend with:

                /**
                 * @property someParam Description of param
                 * @param someProp Description of property
                 */
                class MyClass(someParam: String, val someProp: String)

Should this rule catch when you use @param over a property or @property over a param?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rule will report such case, because @param is strictly required to be param, and @Property to be property. I've added test case for this scenario

@BraisGabin
Copy link
Member

To me this PR is ready to merge. I would like to know your opinion about my comments but both can be done in a follow up PR.

The only thing stopping us to merge this is that CI is complaining. Could you check those issues?

@lukaszosowicki
Copy link
Contributor Author

lukaszosowicki commented Oct 19, 2021

I hopefully fixed CI (there were some minors e.g. not compiling test snipets, because of lack of function body).

I would also like to fix issue mentioned here #4185 (comment) before you merge
[Edit] Ok, I had a moment and I did it right away :)

@BraisGabin
Copy link
Member

Great first contribution 👏

@BraisGabin BraisGabin merged commit 844a971 into detekt:main Oct 19, 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