refactor: reduce context.Background() #3888
Conversation
ashwat287
force-pushed
the
RefactorContextBG
branch
3 times, most recently
from
d321bbf to
1566004
Compare
cmd/limactl/clone.go
Outdated
| @@ -71,7 +71,7 @@ func cloneAction(cmd *cobra.Command, args []string) error { | |||
| if err != nil { | |||
| return err | |||
| } | |||
| yBytes, err := yqutil.EvaluateExpression(yq, yContent) | |||
| yBytes, err := yqutil.EvaluateExpression(cmd.Context(), yq, yContent) | |||
There was a problem hiding this comment.
| yBytes, err := yqutil.EvaluateExpression(cmd.Context(), yq, yContent) | |
| yBytes, err := yqutil.EvaluateExpression(ctx, yq, yContent) |
Same for other occurrences of cmd.Context() too
ashwat287
force-pushed
the
RefactorContextBG
branch
from
1566004 to
5426244
Compare
There was a problem hiding this comment.
Thanks, LGTM
I have only 2 stylistic nit-picks that you can address, if you want. But we already have existing functions that use _ for the single unnamed parameter, so it isn't a new issue.
There is of course the issue of failing CI, but I think this might be GitHub flakiness; we'll see...
pkg/driver/driver.go
Outdated
| @@ -63,7 +63,7 @@ type Registration interface { | |||
| // GuestAgent defines operations for the guest agent. | |||
| type GuestAgent interface { | |||
| // ForwardGuestAgent returns if the guest agent sock needs forwarding by host agent. | |||
| ForwardGuestAgent() bool | |||
| ForwardGuestAgent(_ context.Context) bool | |||
There was a problem hiding this comment.
If a function/method has only unnamed parameters, then you don't need to specify the _ name. It is only needed when you want to provide a name for parameters with a basic type like bool. int, or string, which are not self-explanatory because you cannot mix named and unnamed parameters.
| ForwardGuestAgent(_ context.Context) bool | |
| ForwardGuestAgent(context.Context) bool |
There was a problem hiding this comment.
Shall we raise a new Issue/PR focusing on removing _ for single unnamed parameter to make it consistent?
pkg/hostagent/hostagent.go
Outdated
| @@ -585,19 +585,20 @@ func (a *HostAgent) watchGuestAgentEvents(ctx context.Context) { | |||
| remoteUnix := "/run/lima-guestagent.sock" | |||
|
|
|||
| a.onClose = append(a.onClose, func() error { | |||
| ctxBG := context.Background() | |||
There was a problem hiding this comment.
I feel like this is a premature optimization. You introduce an extra variable name that the user needs to keep track of instead of calling a very cheap function multiple times.
It doesn't matter much, but I would always try to minimize the context that the developer needs to carry in their head while reading the code, especially if it is a rather long function.
There was a problem hiding this comment.
Yeah, makes sense—I'll drop ctxBG and just call context.Background() directly.
ashwat287
force-pushed
the
RefactorContextBG
branch
from
5426244 to
6a5b683
Compare
|
@ashwat287, please fix the merge conflicts |
ashwat287
force-pushed
the
RefactorContextBG
branch
5 times, most recently
from
b9c9050 to
fffdae1
Compare
pkg/hostagent/dns/dns.go
Outdated
| type DNSHandler interface { | ||
| ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) | ||
| } | ||
|
|
||
| type DNSServer struct { | ||
| Server dns.Server | ||
| DNSHandler DNSHandler | ||
| } | ||
|
|
There was a problem hiding this comment.
Why is this needed? This seems out of context for this PR
ashwat287
force-pushed
the
RefactorContextBG
branch
from
fffdae1 to
75692e7
Compare
Signed-off-by: nerdylucifer <[email protected]>
ashwat287
force-pushed
the
RefactorContextBG
branch
from
75692e7 to
79954fd
Compare
fixes #3875