Exorcise golanci-lint errors #10949
Exorcise golanci-lint errors #10949
Conversation
monitoring/generator.go
Outdated
| if err := o.validate(); err != nil { | ||
| return fmt.Errorf("observable %q: %v", o.Name, err) | ||
| for i := range r { | ||
| if err := r[i].validate(); err != nil { |
There was a problem hiding this comment.
Whoah wait is this really a rule?
There was a problem hiding this comment.
I personally prefer using values instead of pointers where I can but here r is so big golangci-lint complains about the copying.
There was a problem hiding this comment.
Can we just get golangci-lint to ignore these "errors" instead?
They definitely make sense where performance is critical, but here code readability is more important IMO than performance and not using pointers makes it completely clear that "these are not mutable pointer values"
|
See golang/lint#210 for unexported lint I feel like the correct way to do this is to use an interface and create a private method that makes that interface unimplementable... |
|
I think the gsec lints need to be ignored, grafana and prometheus configs needs to be read-able and editable by user 476 (in grafana's case) |
monitoring/generator.go
Outdated
| @@ -189,8 +189,8 @@ type Observable struct { | |||
| // | |||
| PossibleSolutions string | |||
|
|
|||
| // PanelOptions describes some options for how to render the metric in the Grafana panel. | |||
| PanelOptions panelOptions | |||
| // NewPanelOptions describes some options for how to render the metric in the Grafana panel. | |||
There was a problem hiding this comment.
This looks wrong, it no longer starts with the field name. I am surprised the linter didn't complain about this at all.
| // NewPanelOptions describes some options for how to render the metric in the Grafana panel. | |
| // PanelOptions describes some options for how to render the metric in the Grafana panel. |
monitoring/generator.go
Outdated
| if err := o.validate(); err != nil { | ||
| return fmt.Errorf("observable %q: %v", o.Name, err) | ||
| for i := range r { | ||
| if err := r[i].validate(); err != nil { |
There was a problem hiding this comment.
Can we just get golangci-lint to ignore these "errors" instead?
They definitely make sense where performance is critical, but here code readability is more important IMO than performance and not using pointers makes it completely clear that "these are not mutable pointer values"
monitoring/generator.go
Outdated
| } | ||
|
|
||
| func (o Observable) validate() error { | ||
| func (o *Observable) validate() error { |
There was a problem hiding this comment.
Can we make it not complain about the pointers here instead? See my comment in https://github.com/sourcegraph/sourcegraph/pull/10949/files#r429587283
|
I'm a bit confused how golangci-lint runs. When I run it locally in @slimsag PTAL, this should address your comments. It looks like this code is mainly used for generating config so I agree we should prioritize readability |
Fixes https://github.com/sourcegraph/sourcegraph/issues/10915
May still need no-lint comments to address go sec file permission errors