[GitHub] [geode] DonalEvans commented on a change in pull request #6716: GEODE-9388: Implement Radish ZRANGEBYLEX Command

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

DonalEvans commented on a change in pull request #6716:
URL: https://github.com/apache/geode/pull/6716#discussion_r675156262



##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/StringBytesGlossary.java
##########
@@ -175,12 +175,13 @@
   @MakeImmutable
   public static final byte[] bABSTTL = stringToBytes("ABSTTL");
 
-  // AbstractZRangeExecutor
+  // Various ZRangeExecutors
   @MakeImmutable
   public static final byte[] bWITHSCORES = stringToBytes("WITHSCORES");
-
   @MakeImmutable
   public static final byte[] bLIMIT = stringToBytes("LIMIT");
+  public static final byte bPLUS = SIMPLE_STRING_ID; // +

Review comment:
       Prior to the refactor that moved constants out of `Coder`, the approach to naming byte/byte array constants was somewhat inconsistent, with some using the "b" prefix and some not. I can't say why this naming convention was originally chosen (commit history shows that it seems to have been present since Geode was first imported to git back in 2015), but when working on https://issues.apache.org/jira/browse/GEODE-9287, I tried to enforce consistency in all the constant names. I think I was overzealous with the single-byte values though, as you're right, many of them don't use the "b" prefix and conceptually it might make more sense to only use it for byte arrays. I can go through and rename the ones are that currently using it so that they're not if you think that'd improve things.




-- 
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: notifications-unsubscribe@geode.apache.org

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