[GitHub] [lucene] uschindler commented on a change in pull request #34: LUCENE-9856: fail precommit on unused local variables

[GitBox <gi...@apache.org>]

uschindler commented on a change in pull request #34:
URL: https://github.com/apache/lucene/pull/34#discussion_r599417385



##########
File path: lucene/core/src/test/org/apache/lucene/search/TestTotalHits.java
##########
@@ -22,6 +22,7 @@
 
 public class TestTotalHits extends LuceneTestCase {
 
+  @SuppressWarnings("unlikely-arg-type")

Review comment:
       Just for interest: what does this complain about?

##########
File path: lucene/analysis/common/src/test/org/apache/lucene/analysis/core/TestAnalyzers.java
##########
@@ -109,7 +109,9 @@ public void _testStandardConstants() {
     x = StandardTokenizer.HIRAGANA;
     x = StandardTokenizer.KATAKANA;
     x = StandardTokenizer.HANGUL;
+    assert x >= 0;

Review comment:
       Should't this be a real assertTrue() of JUnit?

##########
File path: lucene/core/src/test/org/apache/lucene/store/TestMultiMMap.java
##########
@@ -181,7 +181,7 @@ public void testSeekZero() throws Exception {
 
   public void testSeekSliceZero() throws Exception {
     int upto = TEST_NIGHTLY ? 31 : 3;
-    for (int i = 0; i < 3; i++) {
+    for (int i = 0; i < upto; i++) {

Review comment:
       woah, thanks for fixing!

##########
File path: lucene/analysis/common/src/java/org/apache/lucene/analysis/miscellaneous/ConcatenateGraphFilter.java
##########
@@ -348,7 +348,7 @@ private static Automaton replaceSep(Automaton a, Character tokenSeparator) {
    * @lucene.internal
    */
   public static final class BytesRefBuilderTermAttributeImpl extends AttributeImpl
-      implements BytesRefBuilderTermAttribute, TermToBytesRefAttribute {
+      implements BytesRefBuilderTermAttribute {

Review comment:
       LOL

##########
File path: lucene/analysis/common/src/test/org/apache/lucene/analysis/cjk/TestCJKBigramFilterFactory.java
##########
@@ -56,11 +56,10 @@ public void testHanOnlyUnigrams() throws Exception {
 
   /** Test that bogus arguments result in exception */
   public void testBogusArguments() throws Exception {
-    IllegalArgumentException expected =

Review comment:
       I think those are indeed unused, but the reason why they are there is a different one: This allows step-by-step debugging. By declaring an unused variable, you can step through the code / set breakpoints and see what the variable contains. Without it, most debuggers don't show the return value (depend on your IDE).
   
   I agree to remove them, but this may annoy people.

##########
File path: gradle/generation/javacc.gradle
##########
@@ -95,6 +95,12 @@ def commonCleanups = { FileTree generatedFiles ->
       text = text.replace(
           "public  void setDebugStream(java.io.PrintStream ds) { debugStream = ds; }",
           "// (setDebugStream omitted).")
+      text = text.replace(
+          "public class QueryParserTokenManager ",
+          "@SuppressWarnings(\"unused\") public class QueryParserTokenManager ")

Review comment:
       Hi, To get rid of the stupid escapes for quotes, use single quotes around:
   
   ```
   '@SuppressWarnings("unused") public class QueryParserTokenManager'
   ```
   
   IMHO I like single quoes in Groovy more, as they don't do expansion of properties/variables inside. I always use single quotes in Grooy to prevent expansion :-)




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org