Inline telemetry lifecycle event calls#197
Conversation
carole-lavillonniere
force-pushed
the
refact-pr-127
branch
from
2283972 to
adfce43
Compare
carole-lavillonniere
changed the title
anisaoshafi
left a comment
anisaoshafi
left a comment
There was a problem hiding this comment.
Valid changes IMO, looks good 💅🏼
There was a problem hiding this comment.
Thanks @carole-lavillonniere! Looks like a simple refactoring that makes everything a bit tidier! The code now constructs the event structs in-place at the call site, which is much more readable than the previous multi-argument helper functions 👍
Given the goal is to reduce functional over-engineering, one change we can still make in start.go to advance that goal is to remove tel as an argument in all functions that are passed opts StartOptions as arguments, given that the telemetry client is a value of StartOptions.
In the future (outside the scope of this PR), we should further encapsulate Start, Stop and similar container commands into a common struct (like a ContainerService class), and make stable dependencies (like the the ContainerRuntime, TelemetryClient, etc...) part of the struct.
| }) | ||
| } | ||
|
|
||
| func validateLicense(ctx context.Context, sink output.Sink, opts StartOptions, tel *telemetry.Client, containerConfig runtime.ContainerConfig, token, licenseFilePath string) error { |
There was a problem hiding this comment.
example where tel can be removed.
| if c == nil { | ||
| return | ||
| } |
There was a problem hiding this comment.
q: under what circumstances could c be nil?
3 participants
Summary
Refactoring addressing suggested improvement #1 from @thrau in this comment #127 (review)
The goal is to make reading the code easier, be more explicit and idiomatic go.
Changes
Move nil safety for
*telemetry.Clientfrom call-site guards into nil receiver methods, and inline the two private helpersemitEmulatorStartError/emitEmulatorStartSuccess.