Conversation
|
also here why not use the caching package? |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! 🚀 New features to boost your workflow:
|
| defer cancel() | ||
| cc, err := grpc.DialContext(ctx, addr, | ||
| grpc.WithBlock(), | ||
| cc, err := grpc.DialContext(ctx, addr, //nolint:staticcheck |
There was a problem hiding this comment.
I have added nolint for this deprecated call for now. At the same time I've opened the issue #377 that is about changing this to the NewClient() call.
There was a problem hiding this comment.
Alright, works for me.
| dialCtx, cancel := context.WithTimeout(ctx, schemaServerConnectRetry) | ||
| defer cancel() | ||
| cc, err := grpc.DialContext(dialCtx, s.config.SchemaServer.Address, opts...) | ||
| cc, err := grpc.DialContext(dialCtx, s.config.SchemaServer.Address, opts...) //nolint:staticcheck |
severindellsperger
left a comment
There was a problem hiding this comment.
I reviewed the changes. Most of it looks good. However, I would check again if the returned errors can be ignored. I prefer a check for the error, or at least a log message, if there was one.
| b, err := opts.Marshal(rsp.ConfigTree) | ||
| if err != nil { | ||
| fmt.Fprintf(os.Stdout, "%v", err) | ||
| _, _ = fmt.Fprintf(os.Stdout, "%v", err) |
There was a problem hiding this comment.
Since you're not using a simple printf, you may want to check the return error of the function at least.
| table.Options(tablewriter.WithAlignment(tw.Alignment{tw.AlignLeft})) | ||
| table.Bulk(toTableData(rsp)) | ||
| table.Render() | ||
| _ = table.Bulk(toTableData(rsp)) |
There was a problem hiding this comment.
Here again, maybe you want to check the returned error here.
| table.Bulk(toTableData(rsp)) | ||
| table.Render() | ||
| _ = table.Bulk(toTableData(rsp)) | ||
| _ = table.Render() |
There was a problem hiding this comment.
Same here (error check).
| table.Options(tablewriter.WithAlignment(tw.Alignment{tw.AlignLeft})) | ||
| table.Bulk(tableData) | ||
| table.Render() | ||
| _ = table.Bulk(tableData) |
| table.Bulk(tableData) | ||
| table.Render() | ||
| _ = table.Bulk(tableData) | ||
| _ = table.Render() |
| }) | ||
|
|
||
| xmlBuilder.AddValue(ctx, &sdcpb.Path{ | ||
| _ = xmlBuilder.AddValue(ctx, &sdcpb.Path{ |
There was a problem hiding this comment.
AddValue seems to be a pretty difficult function that can return different errors. You may process/log them here.
| log := logf.FromContext(ctx).WithName("Get") | ||
| ctx = logf.IntoContext(ctx, log) | ||
| // log := logf.FromContext(ctx).WithName("Get") | ||
| // ctx = logf.IntoContext(ctx, log) |
There was a problem hiding this comment.
Can those comments be removed?
|
|
||
| // Task 1: blocks until cancelled | ||
| vp.SubmitFunc(func(ctx context.Context, submit func(Task) error) error { | ||
| _ = vp.SubmitFunc(func(ctx context.Context, submit func(Task) error) error { |
There was a problem hiding this comment.
Consider checking/logging the returned error if there is any.
|
|
||
| // Task 2: should run even if cancelled (to decrement inflight) | ||
| vp.SubmitFunc(func(ctx context.Context, submit func(Task) error) error { | ||
| _ = vp.SubmitFunc(func(ctx context.Context, submit func(Task) error) error { |
There was a problem hiding this comment.
Consider checking/logging the returned error if there is any.
|
|
||
| // stringToDisk just for debugging purpose | ||
| func (r *RootEntry) stringToDisk(filename string) error { | ||
| func (r *RootEntry) stringToDisk(filename string) error { // nolint:unused |
There was a problem hiding this comment.
Is there no way to solve this differently so we don't have to mention the unused lint check directive?
There was a problem hiding this comment.
you can chnage it to an uppercase Function, but then it is a public function ... and this is really only used by me I guess in debugging to write the tree to a file to be able to inspect it ... because of the limitations that vars are only displayed to a certain length bla bla
I added the lint GitHub action similar to the config-server.
Please review the changes and merge them if they look good to you.
Close #323