-
Notifications
You must be signed in to change notification settings - Fork 409
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix golangci-lint issues and delete unused code. #1924
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we capture err and handle it accordingly wherever we ignore it through _ ?
@@ -602,7 +601,7 @@ func (t *ForeignModsTest) ReadBeyondEndOfFile() { | |||
AssertEq(contents[contentLen-1], buf[0]) | |||
|
|||
if err == nil { | |||
n, err = f.Read(buf) | |||
n, _ = f.Read(buf) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of this should we put Assert(nil, err) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried doing that. Seems like these errors have been deliberately ignored. Adding such asserts causes the tests to fail.
@@ -240,7 +240,7 @@ func (t *AppendObjectCreatorTest) CallsComposeObjectsWithObjectProperties() { | |||
WillOnce(Return(nil)) | |||
|
|||
// Call | |||
t.call() | |||
_, _ = t.call() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we capture err and assert?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Asserting for error not being nil causes tests to fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is err not nil?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it isn't
@@ -332,15 +332,15 @@ const ( | |||
func (tf *tempFile) ensure(limit int64) error { | |||
switch tf.state { | |||
case fileIncomplete: | |||
size, err := tf.f.Seek(0, 2) | |||
size, _ := tf.f.Seek(0, 2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we return err instead of ignoring?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In certain io methods, an EOF is returned as error. So, an error might not always imply a problem. Therefore I'm a bit scared changing this logic as it might lead to behavior changes. WDYT?
Description
Link to the issue in case of a bug fix.
NA
Testing details