new "transform arguments" API for better key and metadata extraction #2733
Conversation
sjpotter
force-pushed
the
cmd-parser-with-commands
branch
3 times, most recently
from
02245f3
to
93f897d
Compare
sjpotter
force-pushed
the
cmd-parser-with-commands
branch
2 times, most recently
from
18e7927
to
2405923
Compare
sjpotter
force-pushed
the
cmd-parser-with-commands
branch
from
2981aad
to
2340c44
Compare
sjpotter
force-pushed
the
cmd-parser-with-commands
branch
from
2340c44
to
c39f377
Compare
| return ['COMMAND', 'GETKEYSANDFLAGS', ...args]; | ||
| parseCommand(parser: CommandParser, args: Array<RedisArgument>) { | ||
| parser.push('COMMAND', 'GETKEYSANDFLAGS'); | ||
| parser.pushVariadic(args); |
There was a problem hiding this comment.
parser.push(...args)
There was a problem hiding this comment.
resolved first comment
| @@ -1,11 +1,13 @@ | |||
| import { CommandParser } from '../client/parser'; | |||
| @@ -1,3 +1,4 @@ | |||
| import { CommandParser } from '../client/parser'; | |||
| @@ -1,10 +1,11 @@ | |||
| import { CommandParser } from '../client/parser'; | |||
|
General Comment, should add to all tests to validate that proper set of key(s) are processed by the parsing. |
| ...key | ||
| ); | ||
| parser.push('KEYS'); | ||
| parser.pushVariadic(key); |
There was a problem hiding this comment.
change to pushKeys
There was a problem hiding this comment.
didn't specify key index before, so makes sense why i didn't use pushKeys, but perhaps it should? or perhaps should never be called from a cluster client?
| } else { | ||
| args.push(key); | ||
| parser.push(key); |
There was a problem hiding this comment.
change to pushKey()
There was a problem hiding this comment.
didn't specify key index before, so makes sense why i didn't use pushKeys, but perhaps it should? or perhaps should never be called from a cluster client?
| export function parseXAddArguments( | ||
| optional: RedisArgument | undefined, | ||
| parser: CommandParser, | ||
| key: RedisArgument, |
There was a problem hiding this comment.
ponder optional argument up front if better elsewhere?
There was a problem hiding this comment.
annoying but due to location of NOMKSTREAM (i.e. xadd NOMKSTREAM), I'm not sure of a better way to do it without duplicating code.
sjpotter
force-pushed
the
cmd-parser-with-commands
branch
from
83715a8
to
21ce0cb
Compare
packages/client/lib/client/parser.ts
Outdated
| firstKey: RedisArgument | undefined; | ||
| respVersion: RespVersions; | ||
| preserve: unknown; | ||
| cachable: boolean; |
There was a problem hiding this comment.
get rid of getter on parser, and just put as part of command struct.
There was a problem hiding this comment.
addressed on all core commands, never did parser.setCacheable on module commands, will need to determine which need to be cached.
| @@ -69,7 +72,7 @@ export interface RedisClusterOptions< | |||
| type ClusterCommand< | |||
| NAME extends PropertyKey, | |||
| COMMAND extends Command | |||
| > = COMMAND['FIRST_KEY_INDEX'] extends undefined ? ( | |||
| > = COMMAND['NOT_KEYED_COMMAND'] extends true ? ( | |||
addCommand needs to be there for sendCommand like ability within a multi. If its there, it might as well be used by createCommand() et al, to avoid repeating code. addScript is there (even though only used once), but now made private to keep the logic for bookkeeping near each other.
leibale
changed the title
leibale
changed the title
Builds on #2716