Add SCRIPT SHOW command #2171
Add SCRIPT SHOW command #2171
Conversation
shohamazon
added
python
java/client/src/main/java/glide/api/commands/GenericBaseCommands.java
Outdated
Show resolved
Hide resolved
java/client/src/main/java/glide/api/commands/GenericBaseCommands.java
Outdated
Show resolved
Hide resolved
| @Override | ||
| public CompletableFuture<String> scriptShow(@NonNull String sha1) { | ||
| return commandManager.submitNewCommand( | ||
| ScriptShow, new String[] {sha1}, this::handleStringResponse); |
There was a problem hiding this comment.
Does it return null if no script found? If yes, use another handler
There was a problem hiding this comment.
Oh right valkey-io/valkey#617
Should be added to the doc in all clients
| @ParameterizedTest(autoCloseArguments = false) | ||
| @MethodSource("getClients") | ||
| public void scriptShow_test(BaseClient client) { | ||
| assumeTrue(SERVER_VERSION.isGreaterThanOrEqualTo("7.9.0")); |
There was a problem hiding this comment.
its release candidate so it would be changed when they will actually release it #2168
There was a problem hiding this comment.
You can put version check 8.0 and tests start run on this version once everything implemented
There was a problem hiding this comment.
or make it a constant we just need to update the constant when it's released.
There was a problem hiding this comment.
I have already added this before so it wont make any difference now 🙁
node/src/BaseClient.ts
Outdated
| * See {@link https://valkey.io/commands/script-show} for more details. | ||
| * | ||
| * @param sha1 - The SHA1 digest of the script. | ||
| * @param decoder - (Optional) {@link Decoder} type which defines how to handle the response. If not set, the default decoder from the client config will be used. |
There was a problem hiding this comment.
| * @param decoder - (Optional) {@link Decoder} type which defines how to handle the response. If not set, the default decoder from the client config will be used. | |
| * @param decoder - (Optional) {@link Decoder} type which defines how to handle the response. | |
| * If not set, the {@link BaseClientConfiguration.defaultDecoder|default decoder} will be used. |
There was a problem hiding this comment.
I copied it from another command
There was a problem hiding this comment.
We're going to update it in all commands
There was a problem hiding this comment.
Actually we decided to use DecoderOption, see #2234
will add changelog, we dont have scripts in transaction |
shohamazon
force-pushed
the
script-show
branch
from
89d69ae
to
f077e21
Compare
| @@ -1581,6 +1582,53 @@ public void invokeScript_with_ScriptOptionsGlideString_returns_success() { | |||
| assertEquals(payload, response.get()); | |||
| } | |||
|
|
|||
| /*@SneakyThrows | |||
There was a problem hiding this comment.
can you throw in a comment to explain why this was removed/commented out?
There was a problem hiding this comment.
Enabling this test causes the scan_with_option tests to fail CI.
TODO: fix the scan_with_options tests and remove dependency on dynamic libraries
| @ParameterizedTest(autoCloseArguments = false) | ||
| @MethodSource("getClients") | ||
| public void scriptShow_test(BaseClient client) { | ||
| assumeTrue(SERVER_VERSION.isGreaterThanOrEqualTo("7.9.0")); |
There was a problem hiding this comment.
or make it a constant we just need to update the constant when it's released.
| * ``` | ||
| */ | ||
| public async scriptShow( | ||
| sha1: GlideString, |
There was a problem hiding this comment.
nit: is there a reason why we don't pass the Script object and internally we can call script.getHash()?
There was a problem hiding this comment.
same as script exists, kill ect...
There was a problem hiding this comment.
well... the comment could apply to any of these commands.
There was a problem hiding this comment.
I have no idea 🙉
| public void scriptShow_returns_script_source_glidestring() { | ||
| // setup | ||
| GlideString scriptSource = gs("return { KEYS[1], ARGV[1] }"); | ||
| Script script = mock(Script.class); |
There was a problem hiding this comment.
Remove
Your unit tests are probably failing because of this mock. And script is not needed anywhere.
Signed-off-by: Shoham Elias <[email protected]>
Co-authored-by: Yury-Fridlyand <[email protected]> Signed-off-by: Shoham Elias <[email protected]>
Signed-off-by: Shoham Elias <[email protected]>
Signed-off-by: Shoham Elias <[email protected]>
Signed-off-by: Shoham Elias <[email protected]>
shohamazon
force-pushed
the
script-show
branch
from
f5e149b
to
cb110a0
Compare
Signed-off-by: Shoham Elias <[email protected]>
shohamazon
force-pushed
the
script-show
branch
from
c0cbe05
to
80c2ecd
Compare
Signed-off-by: Shoham Elias <[email protected]>
shohamazon
force-pushed
the
script-show
branch
2 times, most recently
from
8929f01
to
af8f6ec
Compare
Signed-off-by: Shoham Elias <[email protected]>
shohamazon
force-pushed
the
script-show
branch
from
af8f6ec
to
2737476
Compare
TODO: