(#2120) GeoRadius support store and storedist option with params #2157
Conversation
|
LGTM! |
| import redis.clients.jedis.util.SafeEncoder; | ||
|
|
||
| public class GeoRadiusStoreParam extends Params { | ||
| private static final String STORE = "store"; |
There was a problem hiding this comment.
@yangbodong22011 I would have set these (STORE &STOREDIST) as Keywords so you won't have to re-serialize them on each call
There was a problem hiding this comment.
I'm afraid I don't understand what you mean. Can you give an example?
There was a problem hiding this comment.
See redis.clients.jedis.Protocol.Keyword
There was a problem hiding this comment.
@gkorland (Almost?) None of the Params classes use Keyword yet. May be we can create a separate issue to improve this?
| } | ||
|
|
||
| public byte[][] getByteParams(byte[]... args) { | ||
| ArrayList<byte[]> byteParams = new ArrayList<byte[]>(); |
There was a problem hiding this comment.
When is that method used?
There was a problem hiding this comment.
When is that method used?
It is currently useless, I followed GeoRadiusParam.
| } | ||
|
|
||
| public byte[][] getByteKeys(byte[] key) { | ||
| List<byte[]> keys = new LinkedList<>(); |
There was a problem hiding this comment.
I think list it an over kill here, you can easily find if you need an array of 2 or 1
|
|
||
| public GeoRadiusStoreParam store(String key) { | ||
| if (key != null) { | ||
| addParam(STORE, key); |
There was a problem hiding this comment.
why do you need it to be Params? it can only be one of the two options.
Params seems like a waste here.
|
@yangbodong22011 Please bear some patience. I'm sure @gkorland would jump in whenever he gets some free time. BTW, thanks for your enthusiasm! |
|
@sazzad16 OK, I will be patient and thank you very much for your reply. |
Resolves #2120
Different from #2140, it is implemented with params.
Closes #2140