-
Notifications
You must be signed in to change notification settings - Fork 0
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
Conversation
class GetHit implements GetResponse { | ||
GetHit(this._value); | ||
|
||
final List<int> _value; |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lib/src/internal/data_client.dart
Outdated
var request = SetRequest_(); | ||
request.cacheKey = key.toBinary(); | ||
request.cacheBody = value.toBinary(); | ||
request.ttlMilliseconds = (ttlSeconds != null |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 87fb164
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
closes #20