feat: adding list API continued #18
Conversation
| validateCacheName($cacheName); | ||
| validateListName($listName); |
There was a problem hiding this comment.
Don't we want to catch InvalidArgumentError for those?
There was a problem hiding this comment.
We already do! It's an SdkError subclass which is handled in the first catch below.
| validateRange($beginIndex, $count); | ||
| $listEraseRequest = new _ListEraseRequest(); | ||
| $listEraseRequest->setListName($listName); | ||
| if (!is_null($beginIndex) && !is_null($count)) { |
There was a problem hiding this comment.
Do we need this if we are already checking with validateRange?
There was a problem hiding this comment.
Oh I see. Those are optional, and if they're null, delete all the items.
There was a problem hiding this comment.
Yes. This is not a validation of the data. If these values are both supplied we need to erase a range of items and if neither value is supplied we need to erase the entire list. The validation makes sure that one or the other of those is true.
|
|
||
| if (!function_exists('validateRange')) | ||
| { | ||
| function validateRange(?int $beginIndex, ?int $count) |
There was a problem hiding this comment.
Do we want to prevent users from passing count value larger than the actual size of the list??
There was a problem hiding this comment.
Validating that would require us to make a call to get the length of the list, and we don't want to make a round trip to the cache server for client side validation in my opinion. A number larger than the size of the list just deletes the rest of the list from the offset without complaining, so that round trip wouldn't buy us anything anyway. If you strongly disagree (or anyone else reading this disagrees) let me know and we can talk about it.
That's a good thing to add to the tests in case the behavior ever changes . . . I'll do that.
There was a problem hiding this comment.
As I wrote this question, I was thinking the same thing about calling another API to get length will add latency so that's probably not something we want to do.
But I didn't know the behavior you explained when a number larger than the size of the list is passed, and I think it makes sense. and thank you for the explanation!
| @@ -290,6 +314,10 @@ public function value() : string | |||
| return $this->value; | |||
| } | |||
|
|
|||
| public function __toString() | |||
There was a problem hiding this comment.
Is this PHP convention? i.e. Are PHP developers expected to echo cacheGetResponseHit object?
Sorry I misunderstand how __toString() works, but I didn't see any method like this in .NET SDK.
There was a problem hiding this comment.
These are the PHP convention equivalent to the ToString() methods in .NET. I added one to each of the non-incubating SDK response types in momentohq/client-sdk-dotnet#252. Your comment does raise the issue that these are not implemented in our incubating response types . . . I'll add a ticket for that on the .NET repo.
| $this->assertEquals(array_slice($values, 2), $response->asHit()->values()); | ||
| } | ||
|
|
||
| public function testListEraseRange_LargeCountValue() |
There was a problem hiding this comment.
Thank you for adding this test!
tests/Cache/CacheClientTest.php
Outdated
| $this->assertEquals(4, $response->asSuccess()->length()); | ||
|
|
||
| $response = $this->client->listErase($this->TEST_CACHE_NAME, $listName); | ||
| $this->assertNotEmpty($response->asSuccess()); |
There was a problem hiding this comment.
This is also assertNotNull??
Adds the remaining list API operations: pop front, pop back, length, and erase. Also adds
__toString()in the response base class and overrides the base implementation in the response subtypes that hold data.