-
Notifications
You must be signed in to change notification settings - Fork 44
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
RPC:golint fix #714
RPC:golint fix #714
Conversation
Which line ? |
rpc/testservice_test.go
Outdated
@@ -93,8 +93,8 @@ func (s *testService) Rets() (string, error) { | |||
} | |||
|
|||
//lint:ignore ST1008 returns error first on purpose. | |||
func (s *testService) InvalidRets1() (string, error) { | |||
return "", nil | |||
func (s *testService) InvalidRets1() (error, string) { |
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.
ignore the ST1008 check here , and then it won't include in the callbacks
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.
InvalidRets1 is made as wrong format method for testing
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.
So we expect it is wrong
Codecov Report
@@ Coverage Diff @@
## master #714 +/- ##
==========================================
- Coverage 50.14% 49.99% -0.15%
==========================================
Files 421 421
Lines 53533 53385 -148
==========================================
- Hits 26844 26691 -153
- Misses 24609 24620 +11
+ Partials 2080 2074 -6
|
@@ -30,6 +30,8 @@ import ( | |||
"time" | |||
) | |||
|
|||
type contextKey string | |||
|
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 it necessary to use a cname here
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.
yes,it's better not to use string as key in the context.withvalue() ,i thought
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.
OK, it seems from golint rules
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.
It will be better to use name of httpContextKey or rpcContextKey
with more meanings than contextKey
here .
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.
ok,
it avoid key collisions by defining a separate type,although may never happen :)
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.
LGTM
However,i don't get why if i return error last in a method,it will increse the nums the reciever.Types().Method()(including that method),which will cause getting more callbacks