EVAL_RO & EVALSHA_RO commands #2168
Conversation
|
Work in progress. |
| /// <seealso href="https://redis.io/commands/eval_ro"/>, | ||
| /// <seealso href="https://redis.io/commands/evalsha_ro"/> | ||
| /// </remarks> | ||
| RedisResult ScriptEvaluateRO(string script, RedisKey[]? keys = null, RedisValue[]? values = null, CommandFlags flags = CommandFlags.None); |
There was a problem hiding this comment.
naming: we've usually tried to be explicit in the API, to trade brevity for clarity; if this RO is "read only", I suggest ScriptEvaluateReadOnly, although I almost wonder whether a CommandFlags makes sense (probably not)
| @@ -1525,9 +1511,24 @@ public Task<RedisResult> ExecuteAsync(string command, ICollection<object>? args, | |||
| return ExecuteAsync(msg, ResultProcessor.ScriptResult, defaultValue: RedisResult.NullSingle); | |||
| } | |||
|
|
|||
| public RedisResult ScriptEvaluate(string script, RedisKey[]? keys = null, RedisValue[]? values = null, CommandFlags flags = CommandFlags.None) | |||
| { | |||
| var command = ResultProcessor.ScriptLoadProcessor.IsSHA1(script) ? RedisCommand.EVALSHA : RedisCommand.EVAL; | |||
There was a problem hiding this comment.
it feels like there's a lot of duplication going on here; maybe worth combining some of this (in particular the message building) via a private helper API that takes bool readOnly ? also some dup re the catch etc, but: ...
There was a problem hiding this comment.
I also do not like it that way, I tried to optimize this line but the problem is that the lines are not the same, in some of the lines you have to place the commands "EVAL/EVALSHA" and in some the commands "EVAL_RO/EVALSHA_RO"
I didn't understand your suggestion to use private helper API that takes bool readOnly, If this is still relevant despite what I have written I would like for you to elaborate further.
|
There is a lot of errors in the tests file, I guess the problem is with my computer so I try to fix it before I write the tests to the commands I added, while you can review the code. |
|
@shacharPash This may have a better change of being addressed if there were unit tests. WDYT about contributing some? |
|
Hey @mgravell , I added some tests to the commands in this PR, I would appreciate it if you would take a look. |
|
@shacharPash no ask at the moment, @mgravell is going to take a look at if this API surface area makes sense (given it's a subset of existing APIs which are a bit fractured) |
No description provided.