rmuir commented on a change in pull request #590:
URL: https://github.com/apache/lucene/pull/590#discussion_r780223468
##########
File path:
lucene/analysis/common/src/java/org/apache/lucene/analysis/core/DecimalDigitFilter.java
##########
@@ -35,6 +35,7 @@ public DecimalDigitFilter(TokenStream input) {
}
@Override
+ @SuppressWarnings("CharacterGetNumericValue") // we use
Character.isDigit(ch), so it is safe
Review comment:
Please, disable this check from error prone completely. it has 100%
false positives and is useless. There is nothing wrong with this using this
method.
##########
File path:
lucene/analysis/common/src/java/org/apache/lucene/analysis/core/DecimalDigitFilter.java
##########
@@ -35,6 +35,7 @@ public DecimalDigitFilter(TokenStream input) {
}
@Override
+ @SuppressWarnings("CharacterGetNumericValue") // we use
Character.isDigit(ch), so it is safe
Review comment:
I just asked to ban this check because it adds useless noise. I'm not a
fan of these kinds of error-prone checks that have a high false-positive rate.
IMO, if we consider methods a trap, I'd rather use forbidden-apis. And if we
want to lint the code, we should crank up javac and ecj warnings (there are
still stuff there we can enable).
It is unclear to me where error-prone fits in... but let's be selective
about which of its crazy heuristical checks gets applied? If there are ones
that truly find bugs, that's different than if we are just making our build
system annoying and fighting the developers.
##########
File path:
lucene/analysis/common/src/java/org/apache/lucene/analysis/core/DecimalDigitFilter.java
##########
@@ -35,6 +35,7 @@ public DecimalDigitFilter(TokenStream input) {
}
@Override
+ @SuppressWarnings("CharacterGetNumericValue") // we use
Character.isDigit(ch), so it is safe
Review comment:
I'm just arguing that this particular check isn't useful.
It is "dumb" in that it seems to just fail if you use the method at all:
https://github.com/google/error-prone/blob/master/core/src/main/java/com/google/errorprone/bugpatterns/CharacterGetNumericValue.java
It doesn't do any "real analysis" to see if e.g. code is guarded by
`Character.isDigit` check or similar.
Again, if we want to ban methods outright, i'd rather use forbidden APIs. It
is a million times faster, and it runs non-nightly and is more friendly to the
developers. Separately, I don't think this particular method needs to be banned
though.
##########
File path: lucene/core/src/test/org/apache/lucene/util/automaton/TestRegExp.java
##########
@@ -99,6 +99,7 @@ private static int randomInt(int bound) {
return bound == 0 ? 0 : random().nextInt(bound);
}
+ @SuppressWarnings("BareDotMetacharacter")
Review comment:
Should we disable this check? It seems sketchy to me:
http://errorprone.info/bugpattern/BareDotMetacharacter
##########
File path: lucene/core/src/test/org/apache/lucene/util/automaton/TestRegExp.java
##########
@@ -99,6 +99,7 @@ private static int randomInt(int bound) {
return bound == 0 ? 0 : random().nextInt(bound);
}
+ @SuppressWarnings("BareDotMetacharacter")
Review comment:
could we disable the check and just add String.replaceAll to forbidden
instead?
I'd really prefer we implement such checks outside of the
nightly-only-slow-errorprone, if possible. It means less tension between
builds/failures and developers
##########
File path: lucene/core/src/test/org/apache/lucene/util/automaton/TestRegExp.java
##########
@@ -99,6 +99,7 @@ private static int randomInt(int bound) {
return bound == 0 ? 0 : random().nextInt(bound);
}
+ @SuppressWarnings("BareDotMetacharacter")
Review comment:
uhhh, it is definitely related to this issue. this issue is the thing
enabling the new check! so I'm trying to suggest a better implementation vs
turning on an annoyance.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
