Sugar syntax for acquiring a shard and logging the trace loop#123
Sugar syntax for acquiring a shard and logging the trace loop#123aarshkshah1992 wants to merge 3 commits intomasterfrom
Conversation
handlers.go
Outdated
|
|
||
| // LogTraceLoop logs the output of the given trace channel until the given context expires. | ||
| func LogTraceLoop(ctx context.Context, traceCh chan Trace, onDone func()) { | ||
| defer onDone() |
There was a problem hiding this comment.
I'm not sure I understand why you need the onDone method here. Instead of passing this function as a parameter, can't the caller just do:
LogTraceLoop(ctx, ch)
onDone()
There was a problem hiding this comment.
Removed this function completely.
handlers.go
Outdated
| } | ||
|
|
||
| // LogTraceLoop logs the output of the given trace channel until the given context expires. | ||
| func LogTraceLoop(ctx context.Context, traceCh chan Trace, onDone func()) { |
There was a problem hiding this comment.
This doesn't belong here. As a utility, this makes way too many assumptions: log level, message, etc. The leakage is obvious in the fact that you need to provide an onDone function as an argument.
There was a problem hiding this comment.
Removed this.
helpers/shard.go
Outdated
| func AcquireShardSync(ctx context.Context, dagst *dagstore.DAGStore, sk shard.Key) (*dagstore.ShardAccessor, error) { | ||
| ch := make(chan dagstore.ShardResult, 1) | ||
|
|
||
| if err := dagst.AcquireShard(ctx, sk, ch, dagstore.AcquireOpts{}); err != nil { |
There was a problem hiding this comment.
To make this a true utility function with no assumptions built-in, you should pass AcquireOpts as an argument.
shard/key.go
Outdated
| } | ||
|
|
||
| // KeyFromCIDMultihash returns a key based on the multihash of the given cid. | ||
| func KeyFromCIDMultihash(cid cid.Cid) Key { |
There was a problem hiding this comment.
Is it too hard to do this in the application? All other Key* functions are constructors. If anything, I'd make this a KeyFromMultihash and pass in the multihash directly.
There was a problem hiding this comment.
Removed this completely.
|
Review addressed and helper added in #128. |
3 participants
Just some sugar syntax to synchronously acquiring a shard and for logging the trace loop in debug mode.