-
-
Notifications
You must be signed in to change notification settings - Fork 767
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
Add NoNameShadowing rule #3477
Add NoNameShadowing rule #3477
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3477 +/- ##
============================================
- Coverage 80.28% 80.08% -0.20%
- Complexity 2784 2851 +67
============================================
Files 454 457 +3
Lines 8415 8537 +122
Branches 1608 1648 +40
============================================
+ Hits 6756 6837 +81
- Misses 789 793 +4
- Partials 870 907 +37
Continue to review full report at Codecov.
|
NoNameShadowing: | ||
active: false |
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.
+1 for putting this under naming. Thank you!
super.visitProperty(property) | ||
report(property) |
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.
Today I learned: KtProperty
and DestructuringDeclarationEntry
are the sealed types of KtVariableDeclaration
import org.jetbrains.kotlin.resolve.BindingContext | ||
|
||
/** | ||
* Disallows shadowing variable declarations. |
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.
We should elaborate a bit here. I don't know if it's necessary to explain what variable shadowing means. But at least say why it's bad.
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.
val findings = subject.compileAndLintWithContext(env, code) | ||
assertThat(findings).hasSize(1) | ||
assertThat(findings[0]).hasMessage("Name shadowed: k") | ||
} |
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.
Does this implementation check the double it
when you nest two lambdas?
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.
assertThat(findings).hasSize(1) | ||
assertThat(findings[0]).hasMessage("Name shadowed: it") | ||
} | ||
|
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.
it("report shadowing nested lambda implicit 'it' parameter") { | |
val code = """ | |
fun test() { | |
listOf(1).forEach { | |
listOf(2).forEach { | |
} | |
} | |
} | |
""" | |
val findings = subject.compileAndLintWithContext(env, code) | |
assertThat(findings).hasSize(1) | |
assertThat(findings[0]).hasMessage("Name shadowed: it") | |
} | |
Do this have sense?
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.
…t/rules/naming/NoNameShadowingSpec.kt Co-authored-by: Brais Gabín <braisgabin@gmail.com>
report(parameter) | ||
} | ||
|
||
private fun report(declaration: KtNamedDeclaration) { |
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 rename this function. report
is a method of the superclass and is doing the actual issue reporting. Here you're instead checking for diagnostics and eventually reporting.
Fixes #240