Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add unary data client with get/set support #35

Merged
merged 6 commits into from
Dec 21, 2023
Merged

Conversation

bruuuuuuuce
Copy link
Collaborator

@bruuuuuuuce bruuuuuuuce commented Dec 21, 2023

closes #20

@bruuuuuuuce bruuuuuuuce marked this pull request as ready for review December 21, 2023 18:17
class GetHit implements GetResponse {
GetHit(this._value);

final List<int> _value;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I played around with this being our Value type, but this approach seemed to be cleaner. It is a bit strange that we take a Value in as a prop for our key/value, but dart does not support method overloading so I still think this is the most sane approach. Open to other ideas tho

@@ -4,6 +4,7 @@
library;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably not a huge deal, but will we want to change the filename to momento in a future pr so it can say we're importing from momento? or is the package name the repo name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm yea good call, ill make a ticket for this

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#37

lib/src/cache_client.dart Outdated Show resolved Hide resolved
lib/src/internal/data_client.dart Outdated Show resolved Hide resolved
var request = SetRequest_();
request.cacheKey = key.toBinary();
request.cacheBody = value.toBinary();
request.ttlMilliseconds = (ttlSeconds != null
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be better to use Dart Duration instead here? https://api.dart.dev/stable/3.2.3/dart-core/Duration/inMilliseconds.html

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This type comes from the protos itself, so we are not able to use Duration, it needs to be of type int64

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh I meant accept ttl as a Duration (like defaultTtl) then convert it to int64 like below

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, yea I like that

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in 87fb164

Copy link
Collaborator

@anitarua anitarua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@bruuuuuuuce bruuuuuuuce merged commit a632ba9 into main Dec 21, 2023
1 check passed
@bruuuuuuuce bruuuuuuuce deleted the feat/unaryGetSet branch December 21, 2023 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cache Client Unary Get/Set
2 participants