CLOUDP-70054: mongocli atlas|om|cm performanceAdvisor slowQueryLogs ls #447
Conversation
There was a problem hiding this comment.
in general the changes to share share some logic between commands feels like a step backwards
I left a couple of comments on the code smells and how to improve this but I wonder why didn't you follow a similar approach as we've done in other commands where we create a base struct we can embed in all the related commands?
type PerformanceAdvisorOpts struct {
ProcessName string
HostID string
}
// all common functions
// embed
type ListOpts struct {
PerformanceAdvisorOpts
}| func (opts *ListOpts) host() (string, error) { | ||
| if opts.processName == "" { | ||
| return opts.hostID, nil | ||
| func Host(processName, hostID string) (string, error) { |
There was a problem hiding this comment.
the method name and the params doesn't tell me anything, the previous style had the approach of relaying on the state to get the correct value but now that you have to pass to values for the method to pick the correct one this feels awkward so I would get rid of this method and do this in each Run() method
There was a problem hiding this comment.
I will leave this as it was before then so that I don't have to add another param to use opts.store.PerformanceAdvisorSlowQueries(opts.ConfigProjectID(), host, opts.newSlowQueryOptions())
| return func() error { | ||
| if config.Service() == config.CloudService { | ||
| return cmd.MarkFlagRequired(flag.ProcessName) | ||
| } | ||
| return cmd.MarkFlagRequired(flag.HostID) | ||
| } | ||
| } | ||
|
|
||
| // Host returns the correct processName or the hostId in accordance with the service |
There was a problem hiding this comment.
Happy to change the name of this function if you find a better one. I definitely need to read something about naming golang function 😅
There was a problem hiding this comment.
This name works, on what to read, Effective Go, is a great resource and here's a section on naming
https://golang.org/doc/effective_go.html#names
| err := opts.validateProcessName() | ||
| if err != nil { | ||
| return "", err | ||
| } |
There was a problem hiding this comment.
[nit] let's make this a oneliner
| err := opts.validateProcessName() | |
| if err != nil { | |
| return "", err | |
| } | |
| if err := opts.validateProcessName(); err != nil { | |
| return "", err | |
| } |
| return func() error { | ||
| if config.Service() == config.CloudService { | ||
| return cmd.MarkFlagRequired(flag.ProcessName) | ||
| } | ||
| return cmd.MarkFlagRequired(flag.HostID) | ||
| } | ||
| } | ||
|
|
||
| // Host returns the correct processName or the hostId in accordance with the service |
There was a problem hiding this comment.
This name works, on what to read, Effective Go, is a great resource and here's a section on naming
https://golang.org/doc/effective_go.html#names
| opts := new(PerformanceAdvisorOpts) | ||
| opts.ProcessName = tc.input | ||
| got := opts.validateProcessName() | ||
| if !reflect.DeepEqual(tc.want, got) { |
There was a problem hiding this comment.
[nit] this was auto generated but we can use assert which has a better interface for testing
Proposed changes
Jira ticket: CLOUDP-70054
Checklist
make fmtand formatted my codeFurther comments
Atlas
OM