-
Notifications
You must be signed in to change notification settings - Fork 510
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
Report unused imports when parent reference of the static import present #535
Report unused imports when parent reference of the static import present #535
Conversation
import androidx.core.text.toSpannable | ||
|
||
fun foo(text: String): Spannable { | ||
return text.toSpannable() |
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.
This is kind of a funny case because I realized (on 0.34.0) that when I renamed the text
parameter of foo
to anything else, the lint rule no longer reported toSpannable
as unused. In this case it's not even a static import, it's just a function call with a variable that happens to match the name of an extension 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.
Yes, since there was import ending with text.toSpannable
, the code asssumed it to be a static import. 😞
In this PR, in addition to the prev fixes, just made sure parent import (with full package name check) exists.
...uleset-standard/src/main/kotlin/com/pinterest/ktlint/ruleset/standard/NoUnusedImportsRule.kt
Outdated
Show resolved
Hide resolved
944e556
to
778324b
Compare
// Builds a list of imports. Takes care of having aliases in case it is assigned to imports | ||
private fun buildNonRedundantImports(text: String) { | ||
val element = text.split("import").last().trim() | ||
if (element.contains(" as ")) { |
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.
there is an ElementType for the as
keyword: ~.c.i.p.impl.source.tree.LeafPsiElement (AS_KEYWORD) "as"
Is it possible to check that instead? Seems slightly more robust than text matching.
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.
That would help. But in this function, the import is sent as a String. The presence of " as " keyword comes in the subsequent node visits.
private fun buildNonRedundantImports(text: String) { | ||
val element = text.split("import").last().trim() | ||
if (element.contains(" as ")) { | ||
imports.addAll(listOf(element.split(" as ").first().replace("`", "").trim(), element.split("as").last().trim())) |
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.
should that second param be checking for " as "
too?
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.
Pulled the " as "
into a variable so as to avoid misses in check
778324b
to
40b8db8
Compare
Fixes #405
This PR contains the changes done in#437 and in addition to it has the scenario covered that was reported in #531 .
Previously the redundant import did not consider the full import path. Added the check in this PR.
There is another issue reported in #526 . Once the steps to reproduce is given, will add it as a test case in this PR to ensure no other regressions are caused.