Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1221713005:
Add an [immediate] argument to StreamQueue.cancel. (Closed)
|
Created:
5 years, 5 months ago by nweiz Modified:
5 years, 5 months ago CC:
reviews_dartlang.org Base URL:
git@github.com:dart-lang/async.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionAdd an [immediate] argument to StreamQueue.cancel.
Canceling the subscription immediately is useful for situations where
the normal flow of a program is interrupted, for example by a signal on
the command-line.
R=lrn@google.com
Committed: https://github.com/dart-lang/async/commit/ab774d6ecbce5689440dbfc80302c26a898b861b
Patch Set 1 #
Total comments: 7
Messages
Total messages: 13 (2 generated)
nweiz@google.com changed reviewers: + lrn@google.com
LGTM if separate function. https://codereview.chromium.org/1221713005/diff/1/lib/src/stream_queue.dart File lib/src/stream_queue.dart (right): https://codereview.chromium.org/1221713005/diff/1/lib/src/stream_queue.dart#n... lib/src/stream_queue.dart:250: } This is really two different functions dispatched on a boolean - all they share is the first two lines. Maybe it should rather be a Future cancelImmediate(); method on the side? I think I would prefer that.
Message was sent while issue was closed.
Committed patchset #1 (id:1) manually as ab774d6ecbce5689440dbfc80302c26a898b861b (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/1221713005/diff/1/lib/src/stream_queue.dart File lib/src/stream_queue.dart (right): https://codereview.chromium.org/1221713005/diff/1/lib/src/stream_queue.dart#n... lib/src/stream_queue.dart:250: } On 2015/07/08 07:49:22, Lasse Reichstein Nielsen wrote: > This is really two different functions dispatched on a boolean - all they share > is the first two lines. > > Maybe it should rather be a > Future cancelImmediate(); > method on the side? I think I would prefer that. I considered that, but ended up going this direction because of the similarity to cancel({force}) elsewhere. If you think a separate method is better, I'll change it.
Message was sent while issue was closed.
lrn@google.com changed reviewers: + floitsch@google.com
Message was sent while issue was closed.
I still think I prefer it as a separate function, but let's get a third opinion. Florian, what do you think?
Message was sent while issue was closed.
https://codereview.chromium.org/1221713005/diff/1/lib/src/stream_queue.dart File lib/src/stream_queue.dart (right): https://codereview.chromium.org/1221713005/diff/1/lib/src/stream_queue.dart#n... lib/src/stream_queue.dart:226: /// immediately. Any pending events will complete as though the stream had Could you please rewrite the "Any pending events ..." sentence. I had a hard time understanding it. Also don't use future tense. https://codereview.chromium.org/1221713005/diff/1/lib/src/stream_queue.dart#n... lib/src/stream_queue.dart:250: } On 2015/07/09 01:03:19, nweiz wrote: > On 2015/07/08 07:49:22, Lasse Reichstein Nielsen wrote: > > This is really two different functions dispatched on a boolean - all they > share > > is the first two lines. > > > > Maybe it should rather be a > > Future cancelImmediate(); > > method on the side? I think I would prefer that. > > I considered that, but ended up going this direction because of the similarity > to cancel({force}) elsewhere. If you think a separate method is better, I'll > change it. One function looks fine to me.
Message was sent while issue was closed.
On 2015/07/09 09:47:52, floitsch wrote: > One function looks fine to me. Ok, let's keep it then.
Message was sent while issue was closed.
https://codereview.chromium.org/1221713005/diff/1/lib/src/stream_queue.dart File lib/src/stream_queue.dart (right): https://codereview.chromium.org/1221713005/diff/1/lib/src/stream_queue.dart#n... lib/src/stream_queue.dart:226: /// immediately. Any pending events will complete as though the stream had On 2015/07/09 09:47:52, floitsch wrote: > Could you please rewrite the "Any pending events ..." sentence. I had a hard > time understanding it. Can you say more about what you didn't understand? > Also don't use future tense. Done in a separate CL.
Message was sent while issue was closed.
https://codereview.chromium.org/1221713005/diff/1/lib/src/stream_queue.dart File lib/src/stream_queue.dart (right): https://codereview.chromium.org/1221713005/diff/1/lib/src/stream_queue.dart#n... lib/src/stream_queue.dart:226: /// immediately. Any pending events will complete as though the stream had On 2015/07/09 19:43:36, nweiz wrote: > On 2015/07/09 09:47:52, floitsch wrote: > > Could you please rewrite the "Any pending events ..." sentence. I had a hard > > time understanding it. > > Can you say more about what you didn't understand? > > > Also don't use future tense. > > Done in a separate CL. Any pending events complete with a 'closed'-event, as though the stream had closed by itself.
Message was sent while issue was closed.
https://codereview.chromium.org/1221713005/diff/1/lib/src/stream_queue.dart File lib/src/stream_queue.dart (right): https://codereview.chromium.org/1221713005/diff/1/lib/src/stream_queue.dart#n... lib/src/stream_queue.dart:226: /// immediately. Any pending events will complete as though the stream had On 2015/07/13 09:39:38, floitsch wrote: > On 2015/07/09 19:43:36, nweiz wrote: > > On 2015/07/09 09:47:52, floitsch wrote: > > > Could you please rewrite the "Any pending events ..." sentence. I had a hard > > > time understanding it. > > > > Can you say more about what you didn't understand? > > > > > Also don't use future tense. > > > > Done in a separate CL. > > Any pending events complete with a 'closed'-event, as though the stream had > closed by itself. Right, but what about that sentence is confusing?
Message was sent while issue was closed.
On 2015/07/13 20:07:21, nweiz wrote: > https://codereview.chromium.org/1221713005/diff/1/lib/src/stream_queue.dart > File lib/src/stream_queue.dart (right): > > https://codereview.chromium.org/1221713005/diff/1/lib/src/stream_queue.dart#n... > lib/src/stream_queue.dart:226: /// immediately. Any pending events will complete > as though the stream had > On 2015/07/13 09:39:38, floitsch wrote: > > On 2015/07/09 19:43:36, nweiz wrote: > > > On 2015/07/09 09:47:52, floitsch wrote: > > > > Could you please rewrite the "Any pending events ..." sentence. I had a > hard > > > > time understanding it. > > > > > > Can you say more about what you didn't understand? > > > > > > > Also don't use future tense. > > > > > > Done in a separate CL. > > > > Any pending events complete with a 'closed'-event, as though the stream had > > closed by itself. > > Right, but what about that sentence is confusing? When I reviewed the CL the sentence was hard to read for me (and fwiw to others I have asked). I can't tell why, and I provided an alternative that feels easier to read for me. It would be great if you found an even better way of expressing the sentence (maybe even expanding it), but I would be already happy if you took my alternative. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
