-
Notifications
You must be signed in to change notification settings - Fork 447
Conversation
Hi @BrennanConroy, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! The agreement was validated by .NET Foundation and real humans are currently evaluating your PR. TTYL, DNFBOT; |
@@ -0,0 +1,107 @@ | |||
using System; |
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.
Header
public IChannelConnection<Message> Application { get; } | ||
public Task Connected => Connection.Metadata.Get<TaskCompletionSource<bool>>("ConnectedTask").Task; | ||
|
||
public TestClient(IServiceProvider serviceProvider, string format = "json") |
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.
Don't we generally like to avoid default params in constructors?
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, especially in public API. Should be fine in tests though.
return false; | ||
} | ||
|
||
public void Close() |
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.
Why do we have Close and Dispose
await Application.Output.WriteAsync(new Message(buffer, Format.Binary, endOfMessage: true)); | ||
} | ||
|
||
public async Task<T> Read<T>() |
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.
Why do we still need this? Is this the lower level API?
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.
e.g. Client1 broadcasts to all clients, how else do we let Client2 see the message?
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.
Ah yes. That makes sense.
using (message) | ||
{ | ||
var serializer = new JsonSerializer(); | ||
value = serializer.Deserialize<T>(new JsonTextReader(new StreamReader(new MemoryStream(message.Payload.Buffer.ToArray())))); |
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.
Shouldn't this use the the adapter?
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.
Ideally :/
It adds a lot of complexity now.
d7685c6
to
75a8ba3
Compare
🆙 📅 |
@anurse Happy |
Looks good to me. |
d96c617
to
72e7b54
Compare
No description provided.