-
Notifications
You must be signed in to change notification settings - Fork 150
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(redis_client): add logging support #1270
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1270 +/- ##
=======================================
Coverage 98.95% 98.96%
=======================================
Files 179 179
Lines 4703 4744 +41
=======================================
+ Hits 4654 4695 +41
Misses 49 49
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
|
||
final finalValue = await client.get(key: key); // null | ||
assert(finalValue == null, 'Key should not exist.'); | ||
// Send a command to the Redis server. |
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.
Simplified the example
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'm too tired to give you a useful review, but I'm very glad to see this coming together!
@@ -73,153 +141,169 @@ class RedisClient { | |||
/// A completer which completes when the client establishes a connection. | |||
var _connected = Completer<void>(); | |||
|
|||
/// Whether the client is connected. | |||
var _isConnected = false; | |||
|
|||
/// A completer which completes when the client disconnects. | |||
/// Begins in a completed state since the client is initially disconnected. | |||
var _disconnected = Completer<void>()..complete(); |
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 thought member variables were typically declared with types in Dart?
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.
The lint rules we have consistently enforce omitting types if they can be inferred.
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.
A few small comments, but mostly looks good. I definitely think this could be its own package. Maybe dart_redis
or something so people don't think it's related to code push in some way?
]); | ||
if (result is RespBulkString) { | ||
final parts = LineSplitter.split(result.payload ?? ''); | ||
final result = await _client.execute(['JSON.GET', key, r'$']); |
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.
Thoughts on extracting these json method names to consts?
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.
They should only be used in this one spot so I didn't see as much value but happy to do so if you prefer.
final finalValue = await client.get(key: key); // null | ||
assert(finalValue == null, 'Key should not exist.'); | ||
// Execute a command. | ||
await client.execute(['PING']); // PONG | ||
|
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.
Why get rid of the get/set/delete examples?
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.
Just thought the simplest getting started would be best but happy to change it back 🤷
Description
Type of Change