[GitHub] [lucene] mikemccand commented on a diff in pull request #858: LUCENE-10551: try to improve testing of LowercaseAsciiCompression

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

mikemccand commented on code in PR #858:
URL: https://github.com/apache/lucene/pull/858#discussion_r862846952


##########
lucene/core/src/java/org/apache/lucene/util/compress/LowercaseAsciiCompression.java:
##########
@@ -111,14 +111,13 @@ public static boolean compress(byte[] in, int len, byte[] tmp, DataOutput out)
           numExceptions2++;
         }
       }
+
+      // TODO: shouldn't this really be an assert instead?  but then this real "if" triggered
+      // LUCENE-10551 so maybe it should remain a real "if":

Review Comment:
   +1, that logic makes sense to me -- it's an `assert` at heart (its purpose is to find bugs in our code, not let the caller know that something went wrong with their inputs), but for (clearly well-founded!) paranoia and low-cost we upgraded it to a real `if`.
   
   But maybe to make it clear that it is a "paranoid upgraded `assert` flavored `if`" we should throw `AssertionError` instead?  Because the user who opened the issue thought this was a case of not being able to compress the string and we should return `false`, but rather it is a case of bug-in-something.  If they saw the `AssertionError` maybe they would realize it's a bug-in-something (a BIS)?



-- 
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-unsubscribe@lucene.apache.org

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