new linter: defaults #200
new linter: defaults #200
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sivchari The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
approved
JoelSpeed
left a comment
JoelSpeed
left a comment
There was a problem hiding this comment.
Thanks for this one, left some feedback inline
pkg/analysis/defaults/initializer.go
Outdated
| // Initializer returns the AnalyzerInitializer for this | ||
| // Analyzer so that it can be added to the registry. | ||
| func Initializer() initializer.AnalyzerInitializer { | ||
| return initializer.NewInitializer(name, Analyzer, true) |
There was a problem hiding this comment.
I think we want this to be configurable. We probably should align with the optionalorrequired linter in having a way to choose which default marker is preferred at least.
I think we also need a way to avoid the use of omitzero, some folks may not be ready for omitzero in their projects
|
|
||
| // GoodK8sDefaultField uses +k8s:default which is also acceptable. | ||
| // +optional | ||
| // +k8s:default="default-value" |
There was a problem hiding this comment.
How do we handle this in optionalorrequired? I think we prefer a single marker rather than both. Bear in mind that +default and +k8s:default are not semantically equivalent for built-in types. Not all tools support both yet, and I think for some time at least, in K/K we will want to have +default and +k8s:default together.
For CRDs I'd expect them to be exclusive
There was a problem hiding this comment.
Should we have some cases that show what happens for a required marked field? I'd expect we want to suggest that shouldn't have required and switch to optional?
| // NoDefaultField is a regular field without default - should not be flagged. | ||
| // +optional | ||
| NoDefaultField string `json:"noDefaultField,omitempty"` |
There was a problem hiding this comment.
Maybe add a required field without omitempty, should also not be flagged?
| InlineField B `json:",inline"` | ||
|
|
||
| // IgnoredField with json ignore tag should not require omitempty. | ||
| // +optional |
There was a problem hiding this comment.
Maybe remove this too to show it doesn't complain optional is missing?
pkg/analysis/defaults/analyzer.go
Outdated
| } | ||
| } | ||
|
|
||
| func checkDefaultOptional(pass *analysis.Pass, field *ast.Field, markersAccess markershelper.Markers, qualifiedFieldName string) { |
There was a problem hiding this comment.
Should also check that it's not required
pkg/analysis/defaults/analyzer.go
Outdated
| { | ||
| Pos: jsonTagInfo.Pos, | ||
| End: jsonTagInfo.End, | ||
| NewText: fmt.Appendf([]byte{}, "%s,omitempty,omitzero", jsonTagInfo.RawValue), |
There was a problem hiding this comment.
I don't think we want to just apply omitzero.
Omitzero should be applied for structs only
Omitempty for all other types
Plus a configuration to switch structs to omitempty as well for projects supporting older Go versions still
|
/approve cancel Just to prevent an accidental merge while there's feedback outstanding |
sivchari
force-pushed
the
new-linter-defaults
branch
from
c7d211e to
3d678fe
Compare
k8s-ci-robot
added
size/XXL
Signed-off-by: sivchari <shibuuuu5@gmail.com>
sivchari
force-pushed
the
new-linter-defaults
branch
from
3d678fe to
0c23a88
Compare
|
Hi, @JoelSpeed |
|
/hold Same above reason. |
k8s-ci-robot
added
the
do-not-merge/hold
3 participants
Close #22
Implemented new linter
defaults