jsonrpc add id from request to response#742
jsonrpc add id from request to response#742peterbourgon merged 3 commits intogo-kit:masterfrom wangzuo:rpc-response-id-fix
Conversation
|
Thanks for the PR. Can you please add a unit test for the correct behavior? |
|
Not sure why the tests failed (timeout) |
| } | ||
| } | ||
|
|
||
| func TestCanMarshalNullID(t *testing.T) { |
There was a problem hiding this comment.
why a separate test for null? wouldn't this fit the TestCanMarshalID test (adding a case for it)
|
@peterbourgon can you review? i’ll be offline the next 2 weeks. |
peterbourgon
left a comment
peterbourgon
left a comment
There was a problem hiding this comment.
With that one comment addressed, I'm +1
| } else { | ||
| return json.Marshal(id.stringValue) | ||
| } | ||
| } |
There was a problem hiding this comment.
My sense is this would be better expressed without the short-circuit logic, as
switch {
case id.intError == nil && id.floatError != nil:
return json.Marshal(id.intValue)
case id.intError != nil && id.floatError == nil:
return json.Marshal(id.floatValue)
default:
return json.Marshal(id.stringValue)
}What do you think?
There was a problem hiding this comment.
I dont think so, intError and floatError can both be nil
There was a problem hiding this comment.
Yes, if intErr == nil and floatErr == nil, the final default case is triggered.
There was a problem hiding this comment.
For integer value, intErr and floatErr are nil and the default case will return empty string.
There was a problem hiding this comment.
Ah, OK, I understand. Then the original implementation is best.
|
Thank you! |
3 participants
According to https://www.jsonrpc.org/specification:
This pull request add req.ID to response object.