-
Notifications
You must be signed in to change notification settings - Fork 182
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 support to disable symbol via regex #669
Conversation
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.
Two comments on constructors and select qualifiers, rest of the changes look good!
import scala.collection.mutable // ok | ||
|
||
@SuppressWarnings(Array("Disable.ListBuffer")) | ||
@SuppressWarnings(Array("Disable.<init>")) |
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 propose we either
- ignore constructors (skip when name is
<init>
), or - give
Disable.<init>
a more helpful name such asListBuffer.<init>
I vote for the first option since you can't reference a constructor without referencing the enclosing type. Having to disable twice is annoying.
@SuppressWarnings(Array("Disable.mutable")) | ||
@SuppressWarnings(Array("Disable.HashMap")) | ||
@SuppressWarnings(Array("Disable.<init>")) | ||
val abc = new mutable.HashMap[String, String]() |
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.
Intuitively, there is only one symbol being referenced here HashMap.<init>
so I think it's a bit weird that three errors are reported.
I think it would make sense to skip the qualifier of a Term.Select
if it only contains Term.Select
or Term.Name
.
a.b.C // only report on C
d.E // only report on E
f.g(1).h // report on h and g
It should be simple to do this check with something like
def skipQualifier(qual: Term) Boolean = qual match {
case _: Term.Name => true
case Term.Select(q, _) => skipQualifier(q)
}
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.
Thanks for the update @vovapolu , one more question.
@@ -29,12 +29,9 @@ object DisableRegex { | |||
import scala.collection.mutable // ok | |||
|
|||
@SuppressWarnings(Array("Disable.ListBuffer")) | |||
@SuppressWarnings(Array("Disable.<init>")) | |||
val buffer = new ListBuffer[Int]() |
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.
What happens if you have a more complex qualifier in a select?
a.b(1).c
and a
and c
are banned? I would expect there to be two lint errors
a.b(1).c
^
a.b(1).c
^
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 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 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.
Removing
OrphanImplicits
from #635, since it's not going to be merged soon...