Add a message accept encoding header #1002
Conversation
|
|
||
| @Override | ||
| public String toAsciiString(List<String> values) { | ||
| return Joiner.on(',').join(values); |
There was a problem hiding this comment.
Currently comma is prohibited from being returned by toAsciiString(). We need to figure out what we want to do here post-beta.
There was a problem hiding this comment.
Well, comma is required by the spec. it also leaves no room for a space, which http2 does support I think. Im personally okay with taking multiple header values as in, getting a list back out. Also possible is splitting out csv's on parsing metadata.
There was a problem hiding this comment.
Oh, I completely understand that comma is required. But we have to figure out how to do it in our API. Our API currently does allow space.
So the problem is that combining is permitting, but splitting is not. We'll need to figure something out here, but we didn't have enough time before beta.
There was a problem hiding this comment.
@ejona86 IIRC the topic of having multiple entries in metadata for the same key has come up before. Should we just defer until we come up with a general solution? For now, assume there is only one value?
There was a problem hiding this comment.
@nmittler The last time it came up we deferred until after beta, but chose to prohibit comma for the moment. It is time for us to actually decide on a solution (cross-language).
There was a problem hiding this comment.
I looked through RFC 7230 (and 5234) which defines the grammar for header names and values. Strictly speaking, the presence or absence of commas is header field dependent, and not generally required for any header field. I am okay deferring this issue until after beta, but it will still need to be fixed.
|
PTAL |
|
@nmittler Could you take a look as well? I am currently blocked with 3 open PRs. |
| new TestMarshaller<Void>(), | ||
| new TestMarshaller<Void>()); | ||
| final ClientTransport transport = mock(ClientTransport.class); | ||
| ClientTransportProvider provider = new TestClientTransportProvider() { |
There was a problem hiding this comment.
nit: you're overriding the only method ... maybe just new ClientTransportProvider?
There was a problem hiding this comment.
Good point, Done!
|
@carl-mastrangelo sorry for the delay! LGTM |
carl-mastrangelo
force-pushed
the
oops
branch
from
1f6601b
to
4572b19
Compare
No description provided.