Conversation
| rm.AddLink("u4", "g2") | ||
| rm.AddLink("u4", "g3") | ||
| rm.AddLink("g1", "g3") | ||
| _ = rm.AddLink("u1", "g1") |
There was a problem hiding this comment.
Why this kind of change is needed?
There was a problem hiding this comment.
AddLink returns an error. This explicitly indicates the error is being ignored.
Otherwise errcheck via gometalinter produces the following:
rbac/default-role-manager/role_manager_test.go:57:12:warning: error return value not checked (rm.AddLink("u1", "g1")) (errcheck)
There was a problem hiding this comment.
Given it is not a Golang official advice, I tend not to follow it. Because it makes a lot of code look tedious and doesn't help readers to understand the code better. Can you rework the commit by removing such changes? Thanks.
There was a problem hiding this comment.
The Golang official advice would be to actually check each of the errors:
https://github.com/golang/go/wiki/CodeReviewComments#handle-errors
https://github.com/golang/lint
To keep backwards compatibility I took the path of explicitly ignoring the errors.
There was a problem hiding this comment.
Handling each error is not practical. My thought is to keep the code as simple as possible, but leave users the option to make it robust (and complicated at most times).
To keep backwards compatibility I took the path of explicitly ignoring the errors.
Could the commit be improved?
| @@ -286,8 +286,12 @@ func NewRoleManager() rbac.RoleManager { | |||
| return &testCustomRoleManager{} | |||
| } | |||
| func (rm *testCustomRoleManager) Clear() error { return nil } | |||
There was a problem hiding this comment.
Why the below two functions are expanded except this one?
There was a problem hiding this comment.
Either goimports or my IDE expanded the others. I did not manually expand any of those.
|
All other changes are OK for me, just reverting all the |
kenjones-cisco
force-pushed
the
task/lint-fixes
branch
from
73f8f93 to
437f266
Compare
|
@kenjones-cisco LGTM. BTW, can you help me answer the questionnaire here: #79? Thanks. |
3 participants
No description provided.