Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
333683003:
Extract out a StreamManager class from json_rpc.Server. (Closed)
|
Created:
6 years, 6 months ago by nweiz Modified:
6 years, 6 months ago Reviewers:
Bob Nystrom CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionExtract out a StreamManager class from json_rpc.Server.
This can then be used to more easily implement Client.
R=rnystrom@google.com
Committed: https://code.google.com/p/dart/source/detail?r=37773
Patch Set 1 #Patch Set 2 : move withoutJson to StreamManager as well #Patch Set 3 : fix tests #
Total comments: 19
Patch Set 4 : code review #Patch Set 5 : code review #Patch Set 6 : code review #
Messages
Total messages: 8 (0 generated)
https://codereview.chromium.org/333683003/diff/40001/pkg/json_rpc_2/lib/src/s... File pkg/json_rpc_2/lib/src/stream_manager.dart (right): https://codereview.chromium.org/333683003/diff/40001/pkg/json_rpc_2/lib/src/s... pkg/json_rpc_2/lib/src/stream_manager.dart:16: class StreamManager { "manager" doesn't mean anything, and it's hard to really say what this class does. The normal constructor is basically a String<->JSON adapter for a two-way stream. The withoutJson constructor gives you an object that basically just does a bit of error piping in listen(). Maybe "StreamAdapter"? https://codereview.chromium.org/333683003/diff/40001/pkg/json_rpc_2/lib/src/s... pkg/json_rpc_2/lib/src/stream_manager.dart:25: final Stream _input; How about Stream<Object>? https://codereview.chromium.org/333683003/diff/40001/pkg/json_rpc_2/lib/src/s... pkg/json_rpc_2/lib/src/stream_manager.dart:33: final StreamSink _output; Likewise StreamSink<Object>. https://codereview.chromium.org/333683003/diff/40001/pkg/json_rpc_2/lib/src/s... pkg/json_rpc_2/lib/src/stream_manager.dart:41: /// Document input and output, in particular that the data on them is JSON strings. https://codereview.chromium.org/333683003/diff/40001/pkg/json_rpc_2/lib/src/s... pkg/json_rpc_2/lib/src/stream_manager.dart:53: "`$outputName` must be passed."); Instead of this conditional logic, how about adding: StreamManager.fromStreamSink(...) https://codereview.chromium.org/333683003/diff/40001/pkg/json_rpc_2/lib/src/s... pkg/json_rpc_2/lib/src/stream_manager.dart:73: /// responses. Document input and output. https://codereview.chromium.org/333683003/diff/40001/pkg/json_rpc_2/lib/src/s... pkg/json_rpc_2/lib/src/stream_manager.dart:77: StreamManager.withoutJson(this._name, Stream input, String inputName, Stream<Object> and StreamSink<Object>? https://codereview.chromium.org/333683003/diff/40001/pkg/json_rpc_2/lib/src/s... pkg/json_rpc_2/lib/src/stream_manager.dart:87: /// Starts listening to the input stream. Document that an error on the stream is piped to the future, and when the future is closed.
https://codereview.chromium.org/333683003/diff/40001/pkg/json_rpc_2/lib/src/s... File pkg/json_rpc_2/lib/src/stream_manager.dart (right): https://codereview.chromium.org/333683003/diff/40001/pkg/json_rpc_2/lib/src/s... pkg/json_rpc_2/lib/src/stream_manager.dart:16: class StreamManager { On 2014/06/13 17:58:04, Bob Nystrom wrote: > "manager" doesn't mean anything, and it's hard to really say what this class > does. The normal constructor is basically a String<->JSON adapter for a two-way > stream. The withoutJson constructor gives you an object that basically just does > a bit of error piping in listen(). > > Maybe "StreamAdapter"? This class is really just "functionality shared between the client and server", hence the generic name. I think handling a two-way stream is closer to its core purpose than adapting JSON to objects. What do you think about "TwoWayStream"? https://codereview.chromium.org/333683003/diff/40001/pkg/json_rpc_2/lib/src/s... pkg/json_rpc_2/lib/src/stream_manager.dart:25: final Stream _input; On 2014/06/13 17:58:04, Bob Nystrom wrote: > How about Stream<Object>? This is basically a union type--not every type is valid for it. I thought you liked dynamic for that. https://codereview.chromium.org/333683003/diff/40001/pkg/json_rpc_2/lib/src/s... pkg/json_rpc_2/lib/src/stream_manager.dart:41: /// On 2014/06/13 17:58:04, Bob Nystrom wrote: > Document input and output, in particular that the data on them is JSON strings. Done. https://codereview.chromium.org/333683003/diff/40001/pkg/json_rpc_2/lib/src/s... pkg/json_rpc_2/lib/src/stream_manager.dart:53: "`$outputName` must be passed."); On 2014/06/13 17:58:04, Bob Nystrom wrote: > Instead of this conditional logic, how about adding: > > StreamManager.fromStreamSink(...) I think having a single method is cleaner and easier for users. Either way we'll need to do some sort of type-sniffing, since there's no way to express "extends both Stream and StreamSink", and I don't want to get a combinatorial explosion between one-way/two-way and encoded/unencoded constructors. https://codereview.chromium.org/333683003/diff/40001/pkg/json_rpc_2/lib/src/s... pkg/json_rpc_2/lib/src/stream_manager.dart:73: /// responses. On 2014/06/13 17:58:04, Bob Nystrom wrote: > Document input and output. Done. https://codereview.chromium.org/333683003/diff/40001/pkg/json_rpc_2/lib/src/s... pkg/json_rpc_2/lib/src/stream_manager.dart:87: /// Starts listening to the input stream. On 2014/06/13 17:58:04, Bob Nystrom wrote: > Document that an error on the stream is piped to the future, and when the future > is closed. Done.
https://codereview.chromium.org/333683003/diff/40001/pkg/json_rpc_2/lib/src/s... File pkg/json_rpc_2/lib/src/stream_manager.dart (right): https://codereview.chromium.org/333683003/diff/40001/pkg/json_rpc_2/lib/src/s... pkg/json_rpc_2/lib/src/stream_manager.dart:12: /// A class for managing a streams of input messages and a sink for output "streams" -> "stream". https://codereview.chromium.org/333683003/diff/40001/pkg/json_rpc_2/lib/src/s... pkg/json_rpc_2/lib/src/stream_manager.dart:16: class StreamManager { On 2014/06/26 20:43:57, nweiz wrote: > On 2014/06/13 17:58:04, Bob Nystrom wrote: > > "manager" doesn't mean anything, and it's hard to really say what this class > > does. The normal constructor is basically a String<->JSON adapter for a > two-way > > stream. The withoutJson constructor gives you an object that basically just > does > > a bit of error piping in listen(). > > > > Maybe "StreamAdapter"? > > This class is really just "functionality shared between the client and server", > hence the generic name. I think handling a two-way stream is closer to its core > purpose than adapting JSON to objects. What do you think about "TwoWayStream"? How about "BidirectionalStream"? Although that's honestly an oxymoron since the whole point of "stream" is to convey unidirectionality. :-/ "StreamMultiplexer"? https://codereview.chromium.org/333683003/diff/40001/pkg/json_rpc_2/lib/src/s... pkg/json_rpc_2/lib/src/stream_manager.dart:25: final Stream _input; On 2014/06/26 20:43:58, nweiz wrote: > On 2014/06/13 17:58:04, Bob Nystrom wrote: > > How about Stream<Object>? > > This is basically a union type--not every type is valid for it. I thought you > liked dynamic for that. SGTM.
https://codereview.chromium.org/333683003/diff/40001/pkg/json_rpc_2/lib/src/s... File pkg/json_rpc_2/lib/src/stream_manager.dart (right): https://codereview.chromium.org/333683003/diff/40001/pkg/json_rpc_2/lib/src/s... pkg/json_rpc_2/lib/src/stream_manager.dart:12: /// A class for managing a streams of input messages and a sink for output On 2014/06/26 21:40:27, Bob Nystrom wrote: > "streams" -> "stream". Done. https://codereview.chromium.org/333683003/diff/40001/pkg/json_rpc_2/lib/src/s... pkg/json_rpc_2/lib/src/stream_manager.dart:16: class StreamManager { On 2014/06/26 21:40:27, Bob Nystrom wrote: > On 2014/06/26 20:43:57, nweiz wrote: > > On 2014/06/13 17:58:04, Bob Nystrom wrote: > > > "manager" doesn't mean anything, and it's hard to really say what this class > > > does. The normal constructor is basically a String<->JSON adapter for a > > two-way > > > stream. The withoutJson constructor gives you an object that basically just > > does > > > a bit of error piping in listen(). > > > > > > Maybe "StreamAdapter"? > > > > This class is really just "functionality shared between the client and > server", > > hence the generic name. I think handling a two-way stream is closer to its > core > > purpose than adapting JSON to objects. What do you think about "TwoWayStream"? > > How about "BidirectionalStream"? Although that's honestly an oxymoron since the > whole point of "stream" is to convey unidirectionality. :-/ > > "StreamMultiplexer"? I think "Multiplexer" strongly implies combining and splitting or potentially routing messages, rather than just handling bidirectionality.
On 2014/06/26 22:55:47, nweiz wrote: > https://codereview.chromium.org/333683003/diff/40001/pkg/json_rpc_2/lib/src/s... > File pkg/json_rpc_2/lib/src/stream_manager.dart (right): > > https://codereview.chromium.org/333683003/diff/40001/pkg/json_rpc_2/lib/src/s... > pkg/json_rpc_2/lib/src/stream_manager.dart:12: /// A class for managing a > streams of input messages and a sink for output > On 2014/06/26 21:40:27, Bob Nystrom wrote: > > "streams" -> "stream". > > Done. > > https://codereview.chromium.org/333683003/diff/40001/pkg/json_rpc_2/lib/src/s... > pkg/json_rpc_2/lib/src/stream_manager.dart:16: class StreamManager { > On 2014/06/26 21:40:27, Bob Nystrom wrote: > > On 2014/06/26 20:43:57, nweiz wrote: > > > On 2014/06/13 17:58:04, Bob Nystrom wrote: > > > > "manager" doesn't mean anything, and it's hard to really say what this > class > > > > does. The normal constructor is basically a String<->JSON adapter for a > > > two-way > > > > stream. The withoutJson constructor gives you an object that basically > just > > > does > > > > a bit of error piping in listen(). > > > > > > > > Maybe "StreamAdapter"? > > > > > > This class is really just "functionality shared between the client and > > server", > > > hence the generic name. I think handling a two-way stream is closer to its > > core > > > purpose than adapting JSON to objects. What do you think about > "TwoWayStream"? > > > > How about "BidirectionalStream"? Although that's honestly an oxymoron since > the > > whole point of "stream" is to convey unidirectionality. :-/ > > > > "StreamMultiplexer"? > > I think "Multiplexer" strongly implies combining and splitting or potentially > routing messages, rather than just handling bidirectionality. OK, I'm fine with Bidirectional or TwoWay. LGTM.
On 2014/06/26 23:56:36, Bob Nystrom wrote: > On 2014/06/26 22:55:47, nweiz wrote: > > > https://codereview.chromium.org/333683003/diff/40001/pkg/json_rpc_2/lib/src/s... > > File pkg/json_rpc_2/lib/src/stream_manager.dart (right): > > > > > https://codereview.chromium.org/333683003/diff/40001/pkg/json_rpc_2/lib/src/s... > > pkg/json_rpc_2/lib/src/stream_manager.dart:12: /// A class for managing a > > streams of input messages and a sink for output > > On 2014/06/26 21:40:27, Bob Nystrom wrote: > > > "streams" -> "stream". > > > > Done. > > > > > https://codereview.chromium.org/333683003/diff/40001/pkg/json_rpc_2/lib/src/s... > > pkg/json_rpc_2/lib/src/stream_manager.dart:16: class StreamManager { > > On 2014/06/26 21:40:27, Bob Nystrom wrote: > > > On 2014/06/26 20:43:57, nweiz wrote: > > > > On 2014/06/13 17:58:04, Bob Nystrom wrote: > > > > > "manager" doesn't mean anything, and it's hard to really say what this > > class > > > > > does. The normal constructor is basically a String<->JSON adapter for a > > > > two-way > > > > > stream. The withoutJson constructor gives you an object that basically > > just > > > > does > > > > > a bit of error piping in listen(). > > > > > > > > > > Maybe "StreamAdapter"? > > > > > > > > This class is really just "functionality shared between the client and > > > server", > > > > hence the generic name. I think handling a two-way stream is closer to its > > > core > > > > purpose than adapting JSON to objects. What do you think about > > "TwoWayStream"? > > > > > > How about "BidirectionalStream"? Although that's honestly an oxymoron since > > the > > > whole point of "stream" is to convey unidirectionality. :-/ > > > > > > "StreamMultiplexer"? > > > > I think "Multiplexer" strongly implies combining and splitting or potentially > > routing messages, rather than just handling bidirectionality. > > OK, I'm fine with Bidirectional or TwoWay. LGTM. Done.
Message was sent while issue was closed.
Committed patchset #6 manually as r37773 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
