Skip to content
This repository has been archived by the owner on Oct 17, 2024. It is now read-only.

Bug a bug in StreamQueueTransaction.reject(). #22

Merged
merged 1 commit into from
Feb 2, 2017
Merged

Conversation

nex3
Copy link
Contributor

@nex3 nex3 commented Feb 1, 2017

This would throw a StateError if StreamQueue.rest had been called on
one of the transaction's child queues, because that queue didn't expect
to be canceled later on.

This would throw a StateError if StreamQueue.rest had been called on
one of the transaction's child queues, because that queue didn't expect
to be canceled later on.
@nex3 nex3 requested a review from lrhn February 1, 2017 21:49
controller.add(4);
controller.add(5);
});

test("child requests' cancel() may still be called explicitly", () async {
transaction.reject();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated comment: It's probably too late, but could we consider renaming reject to cancel? It's just so much more common, and it was the first name I looked for when I tried using the class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's probably too late. I'm not sure I like cancel anyway—in other contexts it usually refers to finalization and releasing of resources, whereas for a transaction it's much more part of the normal flow of usage.

@lrhn
Copy link
Contributor

lrhn commented Feb 2, 2017

LGTM

@nex3 nex3 merged commit 6dbf465 into master Feb 2, 2017
@nex3 nex3 deleted the fix-rest-bug branch February 2, 2017 21:39
mosuem pushed a commit to dart-lang/core that referenced this pull request Oct 14, 2024
This would throw a StateError if StreamQueue.rest had been called on
one of the transaction's child queues, because that queue didn't expect
to be canceled later on.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

3 participants