-
-
Notifications
You must be signed in to change notification settings - Fork 114
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 issues reported by errcheck #285
Conversation
Please retry analysis of this Pull-Request directly on SonarCloud. |
0f2d78e
to
9017ae0
Compare
server.go
Outdated
s := udp.NewServer(udp.WithMux(handler)) | ||
defer func() { | ||
if err := l.Close(); err != 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.
Wasn't sure how to handle this, so there are three suggestions in this file
- add public
Error
method to tcp/udp.Server that handles errors - use
log.Printf
- ignore it
@jkralik what do you think, which way is the best?
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.
you can handle this error at line 32. If s.Serve
returns nil then you use error from l.Close
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 guess that's alright, we can ignore the Close
error if the Serve
call fails.
@@ -36,8 +35,8 @@ func (m *inactivityMonitor) LastActivity() time.Time { | |||
return time.Time{} | |||
} | |||
|
|||
func CloseClientConn(cc ClientConn) { | |||
cc.Close() | |||
func CloseClientConn(cc ClientConn) error { |
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.
API change, but adding error
output shouldn't break most code (only cases I can think of is assigning to function pointer with previous signature or adhering to interface).
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.
This is concrete default implemenetation. So for default, we can ignore the error.
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.
Actually, func CloseClientConn
is not used anywhere. Do we need it at all? The caller can call Close
on CloseClientConn
directly without this wrapper.
5828baa
to
4fd49ff
Compare
Codecov Report
@@ Coverage Diff @@
## master #285 +/- ##
==========================================
- Coverage 63.82% 63.61% -0.21%
==========================================
Files 76 76
Lines 5708 5755 +47
==========================================
+ Hits 3643 3661 +18
- Misses 1688 1705 +17
- Partials 377 389 +12
Continue to review full report at Codecov.
|
4fd49ff
to
47ea08f
Compare
SonarCloud Quality Gate failed. |
No description provided.