DOC-5399 set cmd examples #3342
Conversation
a-TODO-rov
left a comment
a-TODO-rov
left a comment
There was a problem hiding this comment.
LGTM !
Minor comments.
|
|
||
| @Test | ||
| public void run() { | ||
| RedisClient redisClient = RedisClient.create("redis://localhost:6379"); |
There was a problem hiding this comment.
RedisClient is AutoClosable and can be put inside try() instead calling shutdown() manually.
| // STEP_START sadd | ||
| CompletableFuture<Void> sadd = asyncCommands.sadd("myset", "Hello").thenCompose(r -> { | ||
| System.out.println(r); // >>> 1 | ||
| // REMOVE_START |
There was a problem hiding this comment.
Yeah, it sounds dramatic! :-) It just means that this line shouldn't appear in the published example on the docs page (we avoid showing the users the asserts that we test the examples with).
| .toCompletableFuture(); | ||
| // STEP_END | ||
| // HIDE_START | ||
| sadd.join(); |
There was a problem hiding this comment.
Instead joins i would prefer the approach with CompletableFuture.allOf() like in HashExample
There was a problem hiding this comment.
We originally did this to avoid the code snippets getting executed out-of-order during testing (some of them use results from previous snippets and so tests fail if a snippet executes before another snippet that it depends on). If this is no longer a problem, or doesn't apply in this case then I'll use allOf, as you suggest.
TBH, we might have to revisit this stuff carefully now we've got Jupyter notebooks for some of the examples (via the "Run in browser" link). Dependencies between the cells can be a problem with the notebooks too.
| // REMOVE_START | ||
| // Clean up any existing data | ||
| Mono<Void> cleanup = reactiveCommands.del("myset").then(); | ||
| cleanup.block(); |
There was a problem hiding this comment.
Like in async we can wrap the block operations in Mono.when()
There was a problem hiding this comment.
I think this was the out-of-order execution during testing again, but as I said for async, I'll certainly change this if it's not a problem here.
| }) | ||
| // REMOVE_START | ||
| .thenApply(r -> { | ||
| assertThat(r.stream().sorted().collect(toList()).toString()).isEqualTo("[Hello, World]"); |
There was a problem hiding this comment.
can be replaced with assertThat(r).containsExactlyInAnyOrder("Hello", "World")
There was a problem hiding this comment.
Ah, that's a great tip, thanks! The streams thing borrows from what we were doing with the Jedis examples, but your suggestion is much neater :-)
3 participants
Make sure that:
mvn formatter:formattarget. Don’t submit any formatting related changes.DOC-5399 and DOC-5398.
Lettuce examples for the
SADDandSMEMBERScommand pages.