chore: make `$refreshTtl` optional and re-order arguments for list push and dictionary increment #77

poppoerika · 2022-11-28T18:20:56Z

Made $refreshTtl optional and default to true.
re-ordered arguments for list push and dictionary increment operations so that $trunncateFrontToSie, $truncateBackToSize, and amount are before $refreshTtl.
updated the tests to reflect the above changes.

This PR needs to get merged before working on #54.

Closes #49
Closes #50

…t list push operations and dictionary increment operation; updates tests accordingly

cprice404

@poppoerika if you want to merge this as an incremental PR it looks good to me, but before we close #49 I wanted to make it conform to the pattern we established in the .NET repo, like this:

momentohq/client-sdk-dotnet-incubating#45

poppoerika · 2022-11-29T00:13:52Z

@cprice404
As I implemented CollectionTtl, I wasn't entirely sure when to use withRefreshTtlOnUpdates.
If users want to refresh TTL on updates, it can be achieved by CollectionTtl::of(10) since $refreshTtl = true by default.
Maybe I'm missing something here.

cprice404 · 2022-11-29T18:17:17Z

@cprice404 As I implemented CollectionTtl, I wasn't entirely sure when to use withRefreshTtlOnUpdates. If users want to refresh TTL on updates, it can be achieved by CollectionTtl::of(10) since $refreshTtl = true by default. Maybe I'm missing something here.

Good question. It's just there for API symmetry, in case people want to write very explicit code.

cprice404

i just have a couple of minor nits and then I'm +1. We should probably also get Pete to take a quick peek when he's back tomorrow.

cprice404 · 2022-11-29T18:19:02Z

src/Cache/_ScsDataClient.php

@@ -175,6 +177,14 @@ private function ttlToMillis(?int $ttl = null): int
        return $ttl * 1000;
    }

+    private function returnCollectionTtl($ttl): CollectionTtl


can we annotate tye type of the argument here? I believe it would be ?CollectionTtl?

cprice404 · 2022-11-29T18:20:36Z

src/Requests/CollectionTtl.php

+
+class CollectionTtl
+{
+    private ?int $ttl;


If we are using integers to represent ttls, the variable names need to include the unit for clarity. I think we were using ttlSeconds in the previous code.

…e unit for ttl

cprice404

✨ 🚢 from me!

but let's let pete take a quick peek

pgautier404 · 2022-12-02T18:37:06Z

src/Cache/SimpleCacheClient.php

@@ -143,9 +144,9 @@ public function listErase(string $cacheName, string $listName, ?int $beginIndex
        return $this->dataClient->listErase($cacheName, $listName, $beginIndex, $count);
    }

-    public function dictionarySetField(string $cacheName, string $dictionaryName, string $field, string $value, bool $refreshTtl, ?int $ttlSeconds = null): CacheDictionarySetFieldResponse
+    public function dictionarySetField(string $cacheName, string $dictionaryName, string $field, string $value, CollectionTtl $ttl = null): CacheDictionarySetFieldResponse


I'd prefer the CollectionTtl parameter be explicitly nullable in all of these functions:

Suggested change

public function dictionarySetField(string $cacheName, string $dictionaryName, string $field, string $value, CollectionTtl $ttl = null): CacheDictionarySetFieldResponse

public function dictionarySetField(string $cacheName, string $dictionaryName, string $field, string $value, ?CollectionTtl $ttl = null): CacheDictionarySetFieldResponse

It looks like the null assignment does that implicitly, but I think making it explicit and consistent with the rest of our nullable parameters will make it more readable and maintainable.

pgautier404 · 2022-12-02T18:41:20Z

src/Cache/_ScsDataClient.php

@@ -271,18 +281,19 @@ public function listFetch(string $cacheName, string $listName): CacheListFetchRe
    }

    public function listPushFront(
-        string $cacheName, string $listName, string $value, bool $refreshTtl, ?int $truncateBackToSize = null, ?int $ttlSeconds = null
+        string $cacheName, string $listName, string $value, ?int $truncateBackToSize = null, CollectionTtl $ttl = null


Same as above: please make all the CollectionTtl parameters in this file explicitly nullable ?CollectionTtl instead.

pgautier404 · 2022-12-02T18:45:47Z

src/Requests/CollectionTtl.php

+        return new CollectionTtl(null, true);
+    }
+
+    public static function of(int $ttl): CollectionTtl


I think I'd prefer this parameter to match the constructor and be called $ttlSeconds instead, so the unit is explicit in the parameter name (and in the named parameter label). I know I personally get confused about when we're using seconds vs. millis.

pgautier404

This looks great! I made a couple small suggestions.

…to ?CollectionTtl; chore: rename of param to

pgautier404

This is great, thanks you!

poppoerika added 2 commits November 28, 2022 10:10

chore: make refreshTtl optional and default to true; re-order argumen…

6cc0d26

…t list push operations and dictionary increment operation; updates tests accordingly

fix: tests

2512025

poppoerika requested review from cprice404 and malandis November 28, 2022 18:21

cprice404 previously approved these changes Nov 28, 2022

View reviewed changes

chore: add CollectionTtl class; update tests accordingly

1d5d4ae

poppoerika dismissed cprice404’s stale review via 1d5d4ae November 28, 2022 23:39

poppoerika added 2 commits November 28, 2022 15:59

fix: use CollectionTtl in setAddElement and fix tests

9be7921

fix: CollectionTtl::fromCacheTtl()->withNoRefreshTtlOnUpdates()

2e8af3a

poppoerika requested a review from cprice404 November 29, 2022 00:11

cprice404 reviewed Nov 29, 2022

View reviewed changes

chore: add type for returnCollectionTtl arg and update nate to includ…

d0dea01

…e unit for ttl

poppoerika requested review from pgautier404 and cprice404 November 29, 2022 18:28

cprice404 previously approved these changes Nov 30, 2022

View reviewed changes

pgautier404 reviewed Dec 2, 2022

View reviewed changes

chore: make CollectionTtl param type explicitly nullable by updating …

4f987c0

…to ?CollectionTtl; chore: rename of param to

poppoerika dismissed cprice404’s stale review via 4f987c0 December 2, 2022 19:23

poppoerika requested a review from pgautier404 December 2, 2022 19:23

pgautier404 approved these changes Dec 2, 2022

View reviewed changes

poppoerika merged commit b067f00 into main Dec 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: make `$refreshTtl` optional and re-order arguments for list push and dictionary increment #77

chore: make `$refreshTtl` optional and re-order arguments for list push and dictionary increment #77

poppoerika commented Nov 28, 2022

cprice404 left a comment

poppoerika commented Nov 29, 2022

cprice404 commented Nov 29, 2022

cprice404 left a comment

cprice404 Nov 29, 2022

cprice404 Nov 29, 2022

cprice404 left a comment

pgautier404 Dec 2, 2022

pgautier404 Dec 2, 2022

pgautier404 Dec 2, 2022

pgautier404 left a comment

pgautier404 left a comment

	public function dictionarySetField(string $cacheName, string $dictionaryName, string $field, string $value, CollectionTtl $ttl = null): CacheDictionarySetFieldResponse
	public function dictionarySetField(string $cacheName, string $dictionaryName, string $field, string $value, ?CollectionTtl $ttl = null): CacheDictionarySetFieldResponse

chore: make $refreshTtl optional and re-order arguments for list push and dictionary increment #77

chore: make $refreshTtl optional and re-order arguments for list push and dictionary increment #77

Conversation

poppoerika commented Nov 28, 2022

cprice404 left a comment

Choose a reason for hiding this comment

poppoerika commented Nov 29, 2022

cprice404 commented Nov 29, 2022

cprice404 left a comment

Choose a reason for hiding this comment

cprice404 Nov 29, 2022

Choose a reason for hiding this comment

cprice404 Nov 29, 2022

Choose a reason for hiding this comment

cprice404 left a comment

Choose a reason for hiding this comment

pgautier404 Dec 2, 2022

Choose a reason for hiding this comment

pgautier404 Dec 2, 2022

Choose a reason for hiding this comment

pgautier404 Dec 2, 2022

Choose a reason for hiding this comment

pgautier404 left a comment

Choose a reason for hiding this comment

pgautier404 left a comment

Choose a reason for hiding this comment

chore: make `$refreshTtl` optional and re-order arguments for list push and dictionary increment #77

chore: make `$refreshTtl` optional and re-order arguments for list push and dictionary increment #77