-
Notifications
You must be signed in to change notification settings - Fork 39
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
NullPointerException on KDOC inspection #1334
Conversation
### What's done: * fixed NullPointerException on KDOC inspection Closes #1331
@@ -88,7 +88,8 @@ class KdocMethods(configRules: List<RulesConfig>) : DiktatRule( | |||
val config = configRules.getCommonConfiguration() | |||
val filePath = node.getFilePath() | |||
val isTestMethod = node.hasTestAnnotation() || isLocatedInTest(filePath.splitPathToDirs(), config.testAnchors) | |||
if (!isTestMethod && !node.isStandardMethod() && !node.isSingleLineGetterOrSetter()) { | |||
if (!isTestMethod && !node.isStandardMethod() && !node.isSingleLineGetterOrSetter() && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please cover with tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Added
@@ -88,7 +88,8 @@ class KdocMethods(configRules: List<RulesConfig>) : DiktatRule( | |||
val config = configRules.getCommonConfiguration() | |||
val filePath = node.getFilePath() | |||
val isTestMethod = node.hasTestAnnotation() || isLocatedInTest(filePath.splitPathToDirs(), config.testAnchors) | |||
if (!isTestMethod && !node.isStandardMethod() && !node.isSingleLineGetterOrSetter()) { | |||
if (!isTestMethod && !node.isStandardMethod() && !node.isSingleLineGetterOrSetter() && | |||
node.treeParent.findAllDescendantsWithSpecificType(ElementType.FUNCTION_TYPE).isEmpty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure that there is node.treeParent
exists?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
Codecov Report
@@ Coverage Diff @@
## master #1334 +/- ##
============================================
+ Coverage 83.49% 83.52% +0.03%
- Complexity 2561 2563 +2
============================================
Files 106 106
Lines 7275 7278 +3
Branches 2014 2017 +3
============================================
+ Hits 6074 6079 +5
+ Misses 354 353 -1
+ Partials 847 846 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
ceb5433
to
6237abe
Compare
diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/smoke/DiktatSmokeTest.kt
Outdated
Show resolved
Hide resolved
diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/chapter5/AvoidNestedFunctionsRule.kt
Outdated
Show resolved
Hide resolved
### What's done: * fixed NullPointerException on KDOC inspection Closes #1331
diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/utils/AstNodeUtils.kt
Outdated
Show resolved
Hide resolved
### What's done: * fixed NullPointerException on KDOC inspection Closes #1331
/** | ||
* Checks if a function is anonymous | ||
*/ | ||
fun ASTNode.isAnonymousFunction() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we have a rule for explicit typing for public function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing explicit type can only mean that the function returns Unit
, so there is no need in such rule. Even in the rule that checks @return
KDoc tag we allow it to be missing if the function has no explicit return type or explicitly specified Unit
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But @Cheshiriks wanted to make a boolean function, no?
He wanted return this.getIdentifierName() == null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
* Checks if a function is anonymous | ||
*/ | ||
fun ASTNode.isAnonymousFunction() { | ||
require(this.elementType == FUN) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can it be a part of checking of this function instead of assumption?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's needed here. We already everywhere have a check that this value is equal to FUN before calling this method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so this function treats to node to be a FUN
. it means that it's can be called only
node.elementType == FUN && node.isAnonymousFunction()
, otherwise -- exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we then move it to KdocMethods.kt
as private method, because it contains a specific logic for your cases only
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's the point. We do not want the developer to try to call this function on a different type of node, In this case, we will not know what to return
package org.cqfn.diktat | ||
|
||
fun foo() { | ||
val sum: (Int, Int, Int,) -> Int = fun( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please also add a unit test (at list for checks), even duplicated. Not only smoke is needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
### What's done: * fixed NullPointerException on KDOC inspection closes #1334
### What's done: * fixed NullPointerException on KDOC inspection closes #1334
### What's done: * fixed NullPointerException on KDOC inspection Closes #1331
### What's done: * fixed NullPointerException on KDOC inspection Closes #1331
* Checks if a function is anonymous | ||
*/ | ||
fun ASTNode.isAnonymousFunction() { | ||
require(this.elementType == FUN) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we then move it to KdocMethods.kt
as private method, because it contains a specific logic for your cases only
This is not quite the correct name of the error. We initially thought that the error was only in the cdoc rules, but it turned out that we didn’t take into account anonymous functions in many places, so it’s wrong to put them only in KdocMethods.kt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approved after adding a KDoc about expectation
What's done:
Closes NullPointerException on KDOC inspection #1331