redis-doc
redis-doc copied to clipboard
redis
fix: revert #927
PR https://github.com/antirez/redis-doc/pull/927 fixed a valid issue, namely allowing nonsensical usage of EX and PX together, but the fix had a worse side-effect (IMO). By setting the type of the expiration argument as type enum with enum values ["EX seconds", "PX milliseconds"], this suggests to clients that the set command can receive an optional argument with literal value 'EX seconds' or 'PX seconds'. In a client library I maintain which generates types from commands.json, this produces incorrect types. Here's a simplified diff (typescript) when updating the redis-doc submodule the client uses to latest master:
- set(key: string, value: string, seconds: ["EX", number]): Promise<string | null>;
- set(key: string, value: string, milliseconds: ["PX", number]): Promise<string | null>;
+ set(key: string, value: string, expiration: "EX seconds" | "PX milliseconds"): Promise<string | null>;
If the library updated to latest master, the new client would actually disallow correct usage. This was formerly a valid call:
client.set("a:foo", "123", ["EX", 60])
With latest master of redis-doc that above receives a compiler error, and it's impossible to use EX or PX without one. It only allows client.set("a:foo": "123", "EX seconds").
A fix for this that didn't allow for set foo bar EX 2 PX 200 type usage would be to allow enum types within tuples, something like:
{
"name": ["expiry", "period"],
"type": [{ "enum": ["EX", "PX"]}, "integer"]
}
I would be happy to discuss adding something like that in a follow-up, but it'd be introducing a new concept, and probably requires more careful thought in order to not create confusion or inconsistency. With this PR, I'd like to just fix a bug, which is that enum is being used incorrectly.
Hello @mmkal
Thanks for submitting another fix to the docs :)
The docs, I believe, are meant to be read by humans. If we merge this PR, the syntax would change from:
SET key value [EX seconds|PX milliseconds] [NX|XX]
to
SET key value [EX seconds] [PX milliseconds] [NX|XX]
implying both could be used together, which could be confusing for the user. Of course, the current syntax is also misleading and some even find the command itself anomalous (https://github.com/antirez/redis/pull/6621).
I would be happy to discuss adding something like that in a follow-up, but it'd be introducing a new concept
The current infrastructure can definitely be improved upon as there are more cases where the DSL we use for documenting the syntax has limitations (ref https://github.com/antirez/redis-io/issues/159).
@itamarhaber thanks for the reply. My feeling was that with the DSL as it is right now the trade-off is worth it, since right now enum is being used differently from every other instance. I added an explicit note to the set.md docs to warn against using EX and PX together - let me know if you think this is good enough, or if you think this should be worked around in downstream clients.
