fix: xreadgroup command keys parsing #31
Conversation
There was a problem hiding this comment.
Thanks a lot for your PR but it does not seem to be exactly correct. The keys are all arguments behind STREAMS until the ID part. From the documentation itself it's not clear if the string ID will be present or not (looking at the example, it's not but that is against the way how the rest of the documentation normally works. I'll have to verify that against Redis itself. If it's not clear what part the "ID" is, it'll be difficult to actually name the keys properly).
Please also check for the linter error. Thanks.
|
Seems like the key indices are in pseudo code: |
add xread keys parsing add tests
|
@BridgeAR thank you for your help. I rewrite it and add some tests. I also add xread command features (basicly the same as xreadgroup) |
| if (args[i].toUpperCase() === 'STREAMS') { | ||
| for (var j = i + 1; j < (args.length - 1); j++) { | ||
| if (String(args[i]).toUpperCase() === 'STREAMS') { | ||
| for (j = i + 1; j <= i + ((args.length - 1 - i) / 2); j++) { |
There was a problem hiding this comment.
Thanks for this!
nitpicky microoptimisation -- for xread you need to start at 0, but for xreadgroup you can skip the GROUP group consumer arguments. This almost certainly doesn't matter, but thought I'd mention it.
There was a problem hiding this comment.
Thank you, I was considering it, but it should be another if or another case... So i left it like it is :-).
There was a problem hiding this comment.
It could be implemented in line 136 as:
for (i = commandName === 'xread' ? 0 : 3; i < args.length - 1; i++) {
| if (args[i].toUpperCase() === 'STREAMS') { | ||
| for (var j = i + 1; j < (args.length - 1); j++) { | ||
| if (String(args[i]).toUpperCase() === 'STREAMS') { | ||
| for (j = i + 1; j <= i + ((args.length - 1 - i) / 2); j++) { |
There was a problem hiding this comment.
It could be implemented in line 136 as:
for (i = commandName === 'xread' ? 0 : 3; i < args.length - 1; i++) {
commands.json
Outdated
| "flags": [ | ||
| "write", | ||
| "noscript", | ||
| "movablekeys" | ||
| ], | ||
| "keyStart": 1, | ||
| "keyStop": 1, | ||
| "step": 1 |
There was a problem hiding this comment.
Would you mind updating the whole commands.json instead of manually updating this entry? That can be done by running npm run build in combination with the newest Redis version.
There was a problem hiding this comment.
Done and done. This is my first contribution ever, thank you for your patience.
|
@borsucok thanks a lot! |
No description provided.