add new rule time equal #584
Conversation
There was a problem hiding this comment.
Hi @sina-devel thanks for the PR!
I've left some comments in the code.
Please add an entry for the new rule in the README.md, and one in the RULES_DESCRIPTIONS.md
| xtyp := l.file.Pkg.TypeOf(expr.X) | ||
| ytyp := l.file.Pkg.TypeOf(expr.Y) | ||
|
|
||
| if !isNamedType(xtyp, "time", "Time") || !isNamedType(ytyp, "time", "Time") { |
There was a problem hiding this comment.
we could add constants for "time" and "Time"
|
|
||
| func (l *lintTimeEqual) Visit(node ast.Node) ast.Visitor { | ||
| expr, ok := node.(*ast.BinaryExpr) | ||
| if !ok { |
There was a problem hiding this comment.
Here we could check if the opperators are == or !=, if is not the case we can return early.
testdata/time-equal.go
Outdated
| t := time.Now() | ||
| u := t | ||
| return t == u // MATCH /use t.Equal(u) instead of "==" operator/ | ||
| } |
There was a problem hiding this comment.
tests for other operators ( !=, <, ...) can be done in a single function
rule/time-equal.go
Outdated
| } | ||
|
|
||
| w := &lintTimeEqual{file, onFailure} | ||
| w.file.Pkg.TypeCheck() |
There was a problem hiding this comment.
TypeCheck might return an error, need to check for it
There was a problem hiding this comment.
Hi, thanks for the comments, I agree but saw that this error has not been checked in other files too.
There was a problem hiding this comment.
this error has not been checked in other files too.
This is something that need to be fixed
There was a problem hiding this comment.
this error has not been checked in other files too.
This is something that need to be fixed
well, i thinks return nil is good , because the Apply method not returns any error.
| @@ -0,0 +1,27 @@ | |||
| package pkg | |||
There was a problem hiding this comment.
After configuring revive to only apply time-equal rule, when doing
./revive -config defaults.toml -formatter friendly testdata/time-equal.go
No failure is found.
This might be due to (silent) type checking errors.
There was a problem hiding this comment.
interesting, it works for me 🤔 😬
Closes #583