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

Change the default propagateCancel argument #213

Merged
merged 3 commits into from
Mar 28, 2022

Conversation

natebosch
Copy link
Contributor

Closes dart-lang/core#352

Authors are more likely to expect propagation, and not cancelling is
often a hidden inefficiency so it often goes unnoticed and untested.

Update the doc comment to focus on the 3 ways an operation can end - as
a value, an error, or a cancellation. This will hopefully make it more
clear that the onCancel callback does not relate to the cancellation
of the returned operation.

Closes #212

Authors are more likely to expect propagation, and not cancelling is
often a hidden inefficiency so it often goes unnoticed and untested.

Update the doc comment to focus on the 3 ways an operation can end - as
a value, an error, or a cancellation. This will hopefully make it more
clear that the `onCancel` callback does not relate to the cancellation
of the returned operation.
@natebosch natebosch requested a review from lrhn March 23, 2022 21:11
/// Creates a new cancelable operation to be completed when this operation
/// completes normally or as an error, or is cancelled.
///
/// If this operation completes normally the [value] is passed to [onValue]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lrhn - WDYT about using the term source operation or original operation? I think returned operation is fairly clear. I intended for the consistent usage of this operation to convey the "this" that the method is called on, but I'm not sure if that comes across.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jakemac53 - you and I had discussed this briefly and I think you were ok with this. Should we consider making it [this] or `this`?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't feel strongly, not sure what [this] would link to and I would only do that if dartdoc understand it and does something sensible, but `this` sgtm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like Dartdoc will link [this] to the class, which is sensible but maybe not useful in this situation.

Analyzer does not have a definition to jump to, though I'd imagine if we consider it important we could make it align with dartdoc.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think just backticks is good then, linking the class isn't very helpful.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say no quoting at all. If you use a quoted `this`, it refers to an object and should be a noun.
In "this operation", the "this" is not a noun. (I don't know the English word for what it is, though. Preposition?)

And using "this operation" is fine with me,\

@natebosch natebosch requested review from jakemac53 and removed request for lrhn March 25, 2022 18:39
Attempt to make the distinction between "this operation" and "returned
operation" stand out more.
@natebosch natebosch merged commit 3f58c32 into master Mar 28, 2022
@natebosch natebosch deleted the cancelable-then-default-propagate branch March 28, 2022 19:28
mosuem pushed a commit to dart-lang/core that referenced this pull request Oct 14, 2024
Closes dart-lang/async#212

Authors are more likely to expect propagation, and not cancelling is
often a hidden inefficiency which may go unnoticed and untested.

Update the doc comment to focus on the 3 ways an operation can end - as
a value, an error, or a cancellation. This will hopefully make it more
clear that the `onCancel` callback does not relate to the cancellation
of the returned operation.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Change the default of propagateCancel argument in CancelableOperation.then
3 participants