Always omit params member from request when empty #67
Conversation
| // don't want this expected error to appear in os.Stderr though when | ||
| // running tests in verbose mode or when other tests fail. | ||
| jsonrpc2.SetLogger(log.New(io.Discard, "", 0)), | ||
| ) |
There was a problem hiding this comment.
This change doesn't actually have anything to do with the rest of this PR. I slipped this in because I kept seeing jsonrpc2: protocol error: invalid character 'i' looking for beginning of value in the test logs while working on tests. I introduced this test in 028a50b (#64) and somehow missed that this test was spamming the test logs.
There was a problem hiding this comment.
I am only noticing now that this line is commented out. Wow, that's sloppy of me. Will fix.
There was a problem hiding this comment.
Fixed in f74a03b.
| } | ||
| // Assert that conn is closed by trying to write to it. | ||
| _, got := conn.Write(nil) | ||
| want := io.ErrClosedPipe | ||
| if got != want { | ||
| t.Fatalf("got %q, want %q", got, want) | ||
| t.Errorf("got %s, want %s", got, want) |
There was a problem hiding this comment.
I switched from t.Fatalf to t.Errorf because this function is being reused in multiple tests. A reusable function such as this one cannot know if it's being used in the main goroutine or in a sub-goroutine. It's invalid to call t.Fatal* in a goroutine other than the main goroutine. Using t.Fatalf in this function hasn't caused any issues (so far), but I figured I would make this change for better hygiene. 🙂
| @@ -32,7 +32,7 @@ type Error struct { | |||
| Data *json.RawMessage `json:"data,omitempty"` | |||
| } | |||
|
|
|||
| // SetError sets e.Data to the JSON representation of v. If JSON | |||
| // SetError sets e.Data to the JSON encoding of v. If JSON | |||
There was a problem hiding this comment.
I tried to use consistent terminology. I personally prefer "encoding" over "representation" solely because it's shorter, but "encoding" also aligns with the terminology used by json.Marshal in the standard library.
With this commit, the JSON encoding of `Request` always omits the params member when calling `Conn.Call`, `Conn.DispatchCall`, or `Conn.Notify` with the `params` argument set to `nil`. This change also removes the `OmitNilParams` call option that was added in commit 8012d49 (sourcegraph#62). As of this commit, if users desire to send a JSON-RPC request with a `params` value of `null`, then they may do so by explicitly setting the `params` argument of `Conn.Call`/`Conn.DispatchCall`/`Conn.Notify` to `json.RawMessage("null")`.
samherrmann
force-pushed
the
omit-empty-params
branch
from
a5ef35b
to
7c2b0a0
Compare
|
I definitely feel like making this change was a good move considering how simple it is for users to send an explicit conn.Call(ctx, method, json.RawMessage("null"), &result) |
There was a problem hiding this comment.
LGTM!!
With this commit, the JSON encoding of Request always omits the params member when calling Conn.Call, Conn.DispatchCall, or Conn.Notify with the params argument set to nil. This change also removes the OmitNilParams call option that was added in commit 8012d49 (#62).
As of this commit, if users desire to send a JSON-RPC request with a params value of null, then they may do so by explicitly setting the params argument of Conn.Call/Conn.DispatchCall/Conn.Notify to json.RawMessage("null").
Can you add either documentation or an example test which demonstrates this?
The line of code changed in this commit was commented out by accident.
|
Once this lands, can we have a |
With this commit, the JSON encoding of
Requestalways omits the params member when callingConn.Call,Conn.DispatchCall, orConn.Notifywith theparamsargument set tonil. This change also removes theOmitNilParamscall option that was added in commit 8012d49 (#62).As of this commit, if users desire to send a JSON-RPC request with a
paramsvalue ofnull, then they may do so by explicitly setting theparamsargument ofConn.Call/Conn.DispatchCall/Conn.Notifytojson.RawMessage("null").