search

uber-archive / multitransport-jsonrpc

JSON-RPC Client (Node.js & Browser) and Server (Node.js) aim at "natural looking" server and client code.
116 stars 22 forks source link

Remove unused property in response object. #61

Closed horiuchi closed 9 years ago

horiuchi commented 9 years ago

I made sure that only one of the result or error will exist. via. http://www.jsonrpc.org/specification#response_object

coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.15%) when pulling ca027996421a72f507bb76d63abe7e1aef5a6083 on horiuchi:response-format into 20b9a7ed3182c6d236cdce64eda527048432ee34 on uber:master.

coveralls commented 9 years ago

Coverage Status

Coverage decreased (-0.77%) when pulling 1903bab6899823f28b69e7afbf01e6dcb454ee3e on horiuchi:response-format into 20b9a7ed3182c6d236cdce64eda527048432ee34 on uber:master.

coveralls commented 9 years ago

Coverage Status

Coverage decreased (-0.58%) when pulling b8b1d609871e4eb83beee3729c6f460f06e2db76 on horiuchi:response-format into 20b9a7ed3182c6d236cdce64eda527048432ee34 on uber:master.

dfellis commented 9 years ago

Sorry for just getting to this. You opened the diff right at the beginning of a major US holiday (Thanksgiving). ;)

A few minor comments. I feel this is definitely the right direction. :)

coveralls commented 9 years ago

Coverage Status

Coverage decreased (-0.73%) when pulling 2ac1355d52b6e02722c88dcfba82f9967d82d4a6 on horiuchi:response-format into 20b9a7ed3182c6d236cdce64eda527048432ee34 on uber:master.

dfellis commented 9 years ago

I'm trying to figure out how it's possible, but somehow the test that hits this codepath is no longer working, but it's tests are passing. We can't merge this until one of us figures out why this regression has occurred.

coveralls commented 9 years ago

Coverage Status

Coverage decreased (-0.77%) when pulling 3178b5460d12f1a80277f8ff3a05548785877766 on horiuchi:response-format into 20b9a7ed3182c6d236cdce64eda527048432ee34 on uber:master.

dfellis commented 9 years ago

@horiuchi I just loaded the latest master and ran the unit tests and those three lines aren't hit on master, either. I have no idea why coveralls is saying they were recently lost.

If you want to re-revert that "change response error format", feel free, I'll merge after your answer either way. :)

horiuchi commented 9 years ago

@dfellis Thank you for checking on master branch. NodeJs may be changed effect?

I think it's better not to change the commit. Please, merge this pull request.

dfellis commented 9 years ago

Alright! Published as 0.8.0 :D

horiuchi commented 9 years ago

:+1: