Adding Jedis HSTRLEN command request #1404 #1408
Conversation
There was a problem hiding this comment.
Thanks for the work @smadasu , I left review comments for the fix, but most of things are from same issue (naming) so you can easily fix it. Other issue is the place for tests.
| @@ -1296,4 +1296,8 @@ public void bitfield(final byte[] key, final byte[]... value) { | |||
| System.arraycopy(value, 0, bitfieldArgs, 1, argsLength); | |||
| sendCommand(Command.BITFIELD, bitfieldArgs); | |||
| } | |||
|
|
|||
| public void hstrlen(final byte[] hashName, final byte[] keyName) { | |||
There was a problem hiding this comment.
http://redis.io/commands/hstrlen
The name for instance is represented via 'key', and the key for hash is represented via 'field'. This is the Redis keyword semantic we need to follow.
| @@ -3731,4 +3731,11 @@ public boolean retainAll(Collection<?> c) { | |||
| client.bitfield(key, arguments); | |||
| return client.getBinaryMultiBulkReply(); | |||
| } | |||
|
|
|||
| @Override | |||
| public Long hstrlen(byte[] hashName, byte[] keyname) { | |||
There was a problem hiding this comment.
Same issue on variable names.
| @@ -1852,4 +1852,14 @@ public Double execute(Jedis connection) { | |||
| } | |||
| }.runBinary(key); | |||
| } | |||
|
|
|||
| @Override | |||
| public Long hstrlen(final byte[] hashName, final byte[] keyName) { | |||
There was a problem hiding this comment.
Same issue on variable names.
| @@ -872,5 +872,11 @@ public Double geodist(byte[] key, byte[] member1, byte[] member2, GeoUnit unit) | |||
| Jedis j = getShard(key); | |||
| return j.bitfield(key, arguments); | |||
| } | |||
|
|
|||
| @Override | |||
| public Long hstrlen(byte[] hashName, byte[] keyName) { | |||
There was a problem hiding this comment.
Same issue on variable names.
| @@ -1212,4 +1212,9 @@ public void bitfield(final String key, final String... arguments) { | |||
| bitfield(SafeEncoder.encode(key), argumentArray); | |||
| } | |||
|
|
|||
| @Override | |||
| public void hstrlen(final String hashName, final String keyName) { | |||
There was a problem hiding this comment.
Same issue on variable names.
| * @param keyName | ||
| * @return lenth of the value for key | ||
| */ | ||
| Long hstrlen(final String hashName, final String keyName); |
There was a problem hiding this comment.
Same issue on variable names.
| * Used for HSTRLEN Redis command | ||
| * @param hashName | ||
| * @param keyName | ||
| * @return lenth of the value for key |
| @@ -219,6 +219,8 @@ | |||
| Response<Long> pfcount(final String key); | |||
|
|
|||
| Response<List<Long>> bitfield(String key, String... arguments); | |||
|
|
|||
| Response<Long> hstrlen(String hashName, String keyName); | |||
There was a problem hiding this comment.
Same issue on variable names.
| jedis.auth("foobared"); | ||
| Jedis jedis = new Jedis("localhost"); |
There was a problem hiding this comment.
I think this is by accident. Could you revert it, or could you explain why this change is necessary?
There was a problem hiding this comment.
Yes. Thats right. I will fix it Thanks.
| @@ -188,4 +188,56 @@ public void testBinaryBitfield() { | |||
| } | |||
| } | |||
|
|
|||
| @Test | |||
There was a problem hiding this comment.
We're placing command tests to other places. I guess you referred above tests but above tests (BitField and BinaryBitField) are also wrong, and so it should be moved, too.
There was a problem hiding this comment.
HashesCommandsTest is the proper place of hash commands.
There was a problem hiding this comment.
One more, you can just use jedis without initialize yourself. Please refer other test methods in HashesCommandsTest.
|
Update parameter names and Test. |
|
Sorry I closed PR by mistake |
|
@smadasu can you please rebase? |
|
New PR after rebasing |
|
Why is it failing just for openjdk7, I don't understand |
|
Travis CI has some bugs with OpenJDK. Maybe we want to get rid of checking OpenJDK. |
|
@smadasu Tests still not moved to proper place. If you want me to move them, please let me know. I'll take care of it. |
|
Sure @HeartSaVioR . Go ahead and clean them up. Thanks for your help. |
|
@HeartSaVioR @smadasu I've re-run the tests for JDK 7 and we're ok now. Sometimes they fail because because of the timeouts. |
|
OK then LGTM from me. |
|
LGTM! |
|
@HeartSaVioR LGTM!. |
Hi,
Here is the pull request for HSTRLEN command