Given that STRALGO LCS can access keys or not, it makes no sense in terms of... well, everything, but we can focus the discussion on ACL if needed.
Suggestion: break compatibility and make it into STRALGO LCSKEYS and STRALGO LCSSTRINGS.
@redis/core-team your thoughts.
Comment From: oranagra
I'm all for it. Hoping no one really uses this command anyway. I'd go even further and say that even though we now have sub commands that have the same capabilities of commands, maybe we can drop the STRALGO, and add the two and individual commands.
Comment From: guybe7
Yes, let's drop STRALGO
LCSKEYS and LCSSTRINGS
Comment From: itamarhaber
Another alternative: STRALGO KEYS LCS and STRALGO STRINGS LCS would also work, w/o removing the command itself.
On Sat, Nov 6, 2021, 15:44 guybe7 @.***> wrote:
Yes, let's drop STRALGO
LCSKEYS and LCSSTRINGS
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/redis/redis/issues/9744#issuecomment-962454020, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABOHL7GCTCR4MXX37EKFHGDUKUWMZANCNFSM5HOGIOTQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.
Disclaimer
The information contained in this communication from the sender is confidential. It is intended solely for use by the recipient and others authorized to receive it. If you are not the recipient, you are hereby notified that any disclosure, copying, distribution or taking action in relation of the contents of this information is strictly prohibited and may be unlawful.
This email has been scanned for viruses and malware, and may have been automatically archived by Mimecast Ltd, an innovator in Software as a Service (SaaS) for business. Providing a safer and more useful place for your human generated data. Specializing in; Security, archiving and compliance. To find out more visit the Mimecast website.
Comment From: yossigo
I also agree, 7.0 is an opportunity to clean this up.
Comment From: guybe7
giving this some extra thought, LCSSTRINGS is not really Redis-y (can't think of any example of data manipulation that isn't executed on keys)
since we are already breaking BC, maybe just have
LCS key1 key2 [LEN] [IDX] [MINMATCHLEN <len>] [WITHMATCHLEN]
or even more flexible
LCS numkeys key1 [key2 ...] [LEN] [IDX] [MINMATCHLEN <len>] [WITHMATCHLEN]
Comment From: madolson
I don't see value in making this change, it seems like change for the same of change.
Comment From: oranagra
LOL. "change for the sake of change"? sounds like the exact same reason this command was introduced in the first place 8-)
p.s. just realized that considering #9733, COMMAND GETKEYS will also error on stralgo lcs strings a b.
so if we keep it, we need to add no-mandatory-keys.
the syntax of this command is all wrong IMHO: * assume all (future) string algorithms will take similar arguments * mix command take takes keys and one that doesn't in the same command. * make it nearly impossible to expose the right key spec in COMMAND INFO (issues cluster clients)
considering that we estimate that no one really uses it, and the fact we're allowed to introduce breaking changes in a major release, i think we may wanna take this opportunity and do our first ever real breakage (apart from Gopher).
actually, i just realized what these two have in common. they're both April fools day project.
Comment From: madolson
"change for the sake of change"? sounds like the exact same reason this command was introduced in the first place 8-)
Lol, so true.
I don't think the command really fits in with the rest of Redis, so I would vote we send it off the same way we did with Gopher if it's really such a problem.