Add notimestamp linter - #105
Conversation
everettraven
left a comment
There was a problem hiding this comment.
Unlike the nophase linter, this linter has a suggested replacement for the substring timestamp (time).
At the very least, we should ensure that we include that replacement suggestion in the report message.
| // First check if the struct field name contains 'timestamp' | ||
| if strings.Contains(strings.ToLower(fieldName), "timestamp") { | ||
| pass.Reportf(field.Pos(), | ||
| "field %s: fields with timestamp substring should be avoided", |
There was a problem hiding this comment.
We should probably include in the message what alternative should be used in place of this. I believe the appropriate replacement here would be time.
Seeing as how this linter has a proposed replacement, should we also implement the suggested fix for this so that if someone were to run lint --fix that it would automatically apply the suggestion?
There was a problem hiding this comment.
hey, A quick question, whats the right way of debugging or to get to know the value of some variable, If I try
fmt.Println(fieldName)
I dont see anything getting printed on the console
There was a problem hiding this comment.
How are you trying to debug it? Running the actual tool or via the tests?
If it is through the tests, you'd want to tell it to be verbose using the -v flag with go test.
There was a problem hiding this comment.
oh ok, I will give a try, Thanks, I was trying with tool
./bin/golangci-kube-api-linter run kube-api-linter/pkg/analysis/notimestamp/testdata/src/a/a.go --fix
9957ad0 to
f751830
Compare
|
Updated the PR to address review comments, Couple of pending questions
twice to fix the names, Once for fixing field names and second time is for fixing json tags. Not sure thats ideal.
|
f751830 to
e462791
Compare
|
|
||
| The `notimestamp` linter checks that the fields in the API types don't contain a 'Timestamp', or any field which contains 'Timestamp' as a substring, e.g CreateTimestamp. |
There was a problem hiding this comment.
Maybe?
| The `notimestamp` linter checks that the fields in the API types don't contain a 'Timestamp', or any field which contains 'Timestamp' as a substring, e.g CreateTimestamp. | |
| The `notimestamp` linter checks that the fields in the API are not named with the word 'Timestamp'. |
What is the reason for this from the API conventions, might be good to help explain that context here a little?
There was a problem hiding this comment.
Is there any other place apart from https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#naming-conventions, To look for reasons for avoiding it.
There was a problem hiding this comment.
I would generally try to use git blame to work out the PR that introduced the rule, and see if there is additional context there. If not, perhaps #api-reviews on the Kuberentes slack might be able to help
There was a problem hiding this comment.
sure thanks, I will check
e462791 to
627e267
Compare
| // Initializer returns the AnalyzerInitializer for this | ||
| // Analyzer so that it can be added to the registry. | ||
| func Initializer() initializer { | ||
| return initializer{} | ||
| } | ||
|
|
||
| // intializer implements the AnalyzerInitializer interface. | ||
| type initializer struct{} | ||
|
|
||
| // Name returns the name of the Analyzer. | ||
| func (initializer) Name() string { | ||
| return name | ||
| } | ||
|
|
||
| // Init returns the intialized Analyzer. | ||
| func (initializer) Init(cfg config.LintersConfig) (*analysis.Analyzer, error) { | ||
| return Analyzer, nil | ||
| } | ||
|
|
||
| // Default determines whether this Analyzer is on by default, or not. | ||
| func (initializer) Default() bool { | ||
| return true | ||
| } |
There was a problem hiding this comment.
A recent refactor has changed the way functions init, can you please rebase and look at another, non-configurable linter to see how this should look now.
commentstart is probably a good example to crib from
There was a problem hiding this comment.
sure will do that
| nofloats.Initializer(), | ||
| nomaps.Initializer(), | ||
| nophase.Initializer(), | ||
| notimestamp.Initializer(), |
There was a problem hiding this comment.
This will conflict when you rebase, we don't maintain this list anymore as each linter now self registers by blank importing in a package called registration
627e267 to
137c091
Compare
9beccf6 to
d805204
Compare
|
Rebased and updated the PR, Ready for review. Thanks. |
|
I think this should be good to go once this last round is resolved, should we squash the commits also? |
193fcb9 to
7606078
Compare
|
Addressed review comments, Added few more fields for testing, Squashed commits. Please take a look. Thanks. |
|
|
||
| if tagReplacementName != tagInfo.Name { | ||
| suggestedFixes = append(suggestedFixes, analysis.SuggestedFix{ | ||
| Message: "replace json tag with Time", |
There was a problem hiding this comment.
Can we include the before and after strings here as we did with the field name please
There was a problem hiding this comment.
oops, Missed it, Updated it now.
|
|
||
| type NoTimeStampTestStruct struct { | ||
| // +optional | ||
| TimeStamp *time.Time `json:"timeStamp,omitempty"` // want "field TimeStamp: prefer use of the term time over timestamp" |
There was a problem hiding this comment.
Does the linter look at the type spec at all? Would a metv1.Time still work?
There was a problem hiding this comment.
Currently the linter does not look at the type spec, on looks for fields with timestamp, wrt to checking type spec. I think we will be covering that in nodurations linter.
7606078 to
b461f66
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JoelSpeed, Karthik-K-N The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Adds a new
notimestamplinter that checks for any fields in struct having timestamp string or substring.Fixes: #26