Support stralgo command #2586
Merged
Support stralgo command #2586
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yangbodong22011
force-pushed
the
feature-stralgo
branch
from
73626e1
to
cb8b382
Compare
yangbodong22011
requested review from
sazzad16,
mina-asham,
dengliming and
gkorland
dengliming
reviewed
Jul 27, 2021
sazzad16
reviewed
Jul 29, 2021
There was a problem hiding this comment.
@yangbodong22011 Sorry, for participating late. But there were several aspects that I wanted to address. So a short comment or review was not an option.
- If the method name is
strAlgoLcs, we already know that the algorithm/subcommand is LCS. AddingStrAlgo algorithmparam is just redundant. - Current
StrAlgoParamsworks for LCS but it may not be convenient in future (if any) algorithms for both users and developers. Naming it(StrAlgo)LCSParamsseems a safer choice. - For same reason,
StringMatchResultmay be better to containLCS. Also put that inresppackage. - How about two different methods
strAlgoLcsandstrAlgoLcsKeys(and the Params withoutkeysoption); so thatstrAlgoLcsKeyscan be used in Cluster as well?
StrAlgoParams for pass params to redis and StringMatchResult is the result
yangbodong22011
force-pushed
the
feature-stralgo
branch
from
cb8b382
to
e2a9bf5
Compare
@sazzad16 @dengliming thanks, all done, pls review again when you have time. |
sazzad16
requested changes
Jul 30, 2021
yangbodong22011
force-pushed
the
feature-stralgo
branch
from
e395b8c
to
2f24a75
Compare
sazzad16
reviewed
Jul 30, 2021
StrAlgoLCSParams
yangbodong22011
force-pushed
the
feature-stralgo
branch
from
2f24a75
to
594362a
Compare
sazzad16
reviewed
Jul 30, 2021
sazzad16
reviewed
Aug 3, 2021
sazzad16
approved these changes
Aug 3, 2021
dengliming
approved these changes
Aug 3, 2021
sazzad16
added a commit
that referenced
this pull request
Aug 7, 2021
* add redis6.0 command `stralgo` support * Support stralgo command StrAlgoParams for pass params to redis and StringMatchResult is the result * Add strAlgoLCSStrings command and remove strings option from StrAlgoLCSParams * Update src/main/java/redis/clients/jedis/resps/LCSMatchResult.java Co-authored-by: ljwlc <liujinwei@inspur.com> Co-authored-by: M Sazzadul Hoque <7600764+sazzad16@users.noreply.github.com>
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Handle #2328 and Resolves #2583