Clean up doc comments for task APIs. #38815

amartini51 · 2021-08-09T22:06:34Z

Use only plain English words in abstracts, no symbol names, per
reference writing guidelines.

Use title case for headings.

Use spelling from Apple Style Guide for 'canceled' and 'canceling'.

Track TaskGroup async(...) renaming to addTask(...) in code examples.

Fill in the -Parameters: markup for methods and initializers that take
parameters, starting the sentence with an upper-case letter.

Add links to TSPL for conceptual discussion, re-using the prose from
elsewhere in the standard library reference for the cross reference.

Restore some prose from fe288c2 in
places where cleanup and revision work to doc comments got overwritten
by API revision.

Restore some prose that was in the discussion of Task.Handle that was
deleted in commit 27bf2c9.

Reduce the number of places where we talk about cooperative cancellation
in detail, and let the individual properties and methods lean on the
longer discussion in the overview for Task.

Remove mention of error propagation from getResult(), which doesn't
throw and therefor can't propagate errors thrown by a task.

Remove mention of what the add(...) methods do if the operation throws
an error from the non-throwing TaskGroup type, since the operation there
can't ever be throwing.

Remove -Returns: markup from properties and from methods that don't
return a value. Stored and computed properties have a value, which is
documented by their abstract and discussion sections; functions and
methods return a value.

Remove cross references when they refer to type that's part of the
declaration. The rendered documentation already provides an affordance
to navigate to a type in that context.

Fixes rdar://81184794

Use only plain English words in abstracts, no symbol names, per reference writing guidelines. Use title case for headings. Use spelling from Apple Style Guide for 'canceled' and 'canceling'. Track TaskGroup async(...) renaming to addTask(...) in code examples. Fill in the -Parameters: markup for methods and initializers that take parameters, starting the sentence with an upper-case letter. Add links to TSPL for conceptual discussion, re-using the prose from elsewhere in the standard library reference for the cross reference. Restore some prose from fe288c2 in places where cleanup and revision work to doc comments got overwritten by API revision. Restore some prose that was in the discussion of Task.Handle that was deleted in commit 27bf2c9. Reduce the number of places where we talk about cooperative cancellation in detail, and let the individual properties and methods lean on the longer discussion in the overview for Task. Remove mention of error propagation from getResult(), which doesn't throw and therefor can't propagate errors thrown by a task. Remove mention of what the add(...) methods do if the operation throws an error from the non-throwing TaskGroup type, since the operation there can't ever be throwing. Remove -Returns: markup from properties and from methods that don't return a value. Stored and computed properties *have* a value, which is documented by their abstract and discussion sections; functions and methods *return* a value. Remove cross references when they refer to type that's part of the declaration. The rendered documentation already provides an affordance to navigate to a type in that context. Fixes <rdar://81184794>.

stdlib/public/Concurrency/Task.swift

ktoso

A few minor things, looks good -- thank you!

stdlib/public/Concurrency/Task.swift

ktoso · 2021-08-10T02:23:49Z

stdlib/public/Concurrency/Task.swift

@@ -122,14 +128,19 @@ extension Task {
    }
  }

-  /// Attempt to cancel the task.
+  /// Indicate that the task should stop.


I don't think "stop" is a safe word to use here, it makes one thing we actively "stop tasks from executing" which isn't the case. It's a bit repetetive but I think we must stick to cancellation wording here, and not introduce new verbs like stopping etc.

Maybe we can brainstorm verbs? The guidelines for writing reference strongly discourage repeating the API symbol name in the abstract (this first line of docs), to avoid the problem of saying things like:

doTheThing() Does the thing.

Where the abstract just repeats in English, using most of the same words, what the symbol name already told you.

I'm particularly hesitant about "attempts" because that sounds to me like the language/framework's implementation of cancellation is only a best effort. We will always tell the task to cancel — whether or not cancellation happens depends on whether that task listens for cancellation, and how that task responds to cancellation.

I see what you mean with it then just repeating the API name... I don't have any other great ideas though :-( Stops makes me pretty worried, we never "stop" anything, the task may continue to run forever for all we know hmm...

You're right on "attempts" that'd be potentially misleading.

Maybe...

Puts the task in cancelled state, for which it may or may not check and act upon.

?

Thanks. How about "Requests that the task should cancel its work."? Or "Indicates that the task should stop running."? I don't like "cancel its work" very much, because it uses "cancel" in a different sort of meaning than we already have in the surrounding reference when we talk about canceling a task.

The fact that a task might ignore cancellation doesn't need to be in the abstract; we discuss it in the paragraphs below.

stdlib/public/Concurrency/Task.swift

stdlib/public/Concurrency/TaskGroup.swift

@ktoso

Feedback from @ktoso.

amartini51 · 2021-08-16T17:20:41Z

@swift-ci Please test

ktoso · 2021-08-17T01:13:57Z

Thanks Alex, nice edits!

I pondered a bit about the cancel docs, let me know what you think but the PR is LGTM 👍

Changes from <rdar://81715064>

This includes: - Edits to /// comments for non-public symbols - Edits to // comments (not documentation) - Edits to warnings and other strings in code

Fixes <rdar://82053644>.

amartini51 · 2021-08-19T06:59:23Z

@swift-ci Please test.

tbkka · 2021-08-19T16:14:06Z

stdlib/public/Concurrency/TaskCancellation.swift

-  // unconditionally add the cancellation record to the task.
-  // if the task was already cancelled, it will be executed right away.
+  // Unconditionally add the cancellation record to the task.
+  // If the task was already canceled, it executes immediately.


"If the task was already canceled, it executes immediately."

This makes it sound like "the task executes immediately", which I presume is not the case for a task that was already canceled.

Nice catch - this is a // comment, not part of the documentation, so should not be changed by these edits. I did a visual check for these for commit 6a79590; and I'll run my usual grep script to check for others now.

Reverted in a0e58c6. @ktoso or @DougGregor do you want to work with @tbkka to improve this internal comment as needed?

@tbkka

Thanks, @tbkka! Confirmed this was the only one by running: git diff github/release/5.5-08092021...HEAD \ | grep '^+\|^-' \ | grep -v '^+++\|^---' \ | grep -v '^- *///\|^+ *///'

amartini51 · 2021-08-20T21:29:45Z

stdlib/public/Concurrency/TaskGroup.swift

+  /// ◊TR: Why do we have two implementations of this method?
+  /// ◊TR: What's the difference between them?


@ktoso or @DougGregor can you comment?

amartini51 · 2021-08-20T21:29:52Z

stdlib/public/Concurrency/TaskGroup.swift

+  /// There are no restrictions on where you can call this method.
+  /// Code inside a child task or even another task can cancel a group.
+  /// ◊TR: Is this rewrite too strong?


@ktoso or @DougGregor can you comment?

amartini51 · 2021-08-20T21:30:35Z

stdlib/public/Concurrency/TaskGroup.swift

+  /// There are no restrictions on where you can call this method.
+  /// Code inside a child task or even another task can cancel a group.
+  /// ◊TR: Is this rewrite too strong?


@ktoso or @DougGregor can you comment?

amartini51 · 2021-08-20T21:30:52Z

stdlib/public/Concurrency/TaskGroup.swift

+  /// ◊TR: Why do we have two implementations of this method?
+  /// ◊TR: What's the difference between them?


@ktoso or @DougGregor can you comment?

amartini51 · 2021-08-20T21:31:02Z

stdlib/public/Concurrency/TaskSleep.swift

+  /// ◊TR: Why do we have both sleep(_:) and sleep(nanoseconds:) -- what's the difference between them?
+  /// ◊TR: It looks like only sleep(nanoseconds:) throws.


@ktoso or @DougGregor can you comment?

I thought we were going to deprecate sleep(_:) so that we could later bring it back when we had a legitimate Duration type. If the argument is just a raw number, then denoting the units as sleep(nanoseconds:) is much clearer.

That sounds right to me, but I don't see an @availability annotation on sleep(_:) that marks it as deprecated here or on the release branch. It sounds like there should be one?

amartini51 · 2021-08-23T20:54:03Z

@swift-ci Please test.

swift-ci · 2021-08-23T21:10:46Z

Build failed
Swift Test OS X Platform
Git Sha - 9b456df

amartini51 · 2021-08-24T16:52:23Z

@swift-ci Please test macOS platform.

Cherry pick #38815, b9dde85.

amartini51 requested review from ktoso and DougGregor August 9, 2021 22:06

ktoso reviewed Aug 10, 2021

View reviewed changes

stdlib/public/Concurrency/Task.swift Show resolved Hide resolved

ktoso reviewed Aug 10, 2021

View reviewed changes

stdlib/public/Concurrency/Task.swift Outdated Show resolved Hide resolved

ktoso reviewed Aug 10, 2021

View reviewed changes

stdlib/public/Concurrency/Task.swift Show resolved Hide resolved

ktoso reviewed Aug 10, 2021

View reviewed changes

stdlib/public/Concurrency/Task.swift Show resolved Hide resolved

ktoso reviewed Aug 10, 2021

View reviewed changes

stdlib/public/Concurrency/Task.swift Outdated Show resolved Hide resolved

ktoso approved these changes Aug 10, 2021

View reviewed changes

amartini51 added 2 commits August 9, 2021 22:27

Incorporate tech review corrections.

946c72c

Feedback from @ktoso.

Match throwing and nonthrowing next() docs.

6492386

amartini51 changed the base branch from release/5.5 to release/5.5-08092021 August 10, 2021 22:23

ghost mentioned this pull request Aug 17, 2021

Sounds good :) itforgeuk/Script-Actions#3

Closed

amartini51 added 6 commits August 18, 2021 23:22

Apply changes from editorial.

1036417

Changes from <rdar://81715064>

Back out edits that are out of scope.

6a79590

This includes: - Edits to /// comments for non-public symbols - Edits to // comments (not documentation) - Edits to warnings and other strings in code

Revise editorial changes.

62968b6

Cross-apply edit: TSPL isn't in italics.

958255d

Use "that" for restrictive clause, per Apple style.

0ab68aa

Remove invalid markup.

e48de1a

Fixes <rdar://82053644>.

amartini51 marked this pull request as ready for review August 19, 2021 06:59

amartini51 requested a review from a team as a code owner August 19, 2021 06:59

tbkka reviewed Aug 19, 2021

View reviewed changes

amartini51 added 2 commits August 19, 2021 10:01

Back out a non-documentation comment change.

a0e58c6

Thanks, @tbkka! Confirmed this was the only one by running: git diff github/release/5.5-08092021...HEAD \ | grep '^+\|^-' \ | grep -v '^+++\|^---' \ | grep -v '^- *///\|^+ *///'

Correct spelling errors and doubled word.

fbcf868

amartini51 commented Aug 20, 2021

View reviewed changes

Remove markers for tech review queries.

9b456df

amartini51 requested a review from a team August 25, 2021 00:15

najacque approved these changes Aug 25, 2021

View reviewed changes

najacque merged commit b9dde85 into swiftlang:release/5.5-08092021 Aug 25, 2021

amartini51 added a commit that referenced this pull request Aug 25, 2021

Clean up doc comments for task APIs.

4bdf869

Cherry pick #38815, b9dde85.

amartini51 mentioned this pull request Aug 25, 2021

Cherry-pick docs changes for Swift 5.5 #39038

Merged

amartini51 mentioned this pull request Nov 10, 2021

Cherry-pick concurrency docs #40115

Merged

amartini51 deleted the docs_task_81184794 branch December 8, 2021 17:48

		/// ◊TR: Why do we have two implementations of this method?
		/// ◊TR: What's the difference between them?

		/// ◊TR: Why do we have both sleep(_:) and sleep(nanoseconds:) -- what's the difference between them?
		/// ◊TR: It looks like only sleep(nanoseconds:) throws.

Clean up doc comments for task APIs. #38815

Clean up doc comments for task APIs. #38815

Uh oh!

Conversation

amartini51 commented Aug 9, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ktoso left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amartini51 Aug 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amartini51 Aug 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amartini51 commented Aug 16, 2021

Uh oh!

ktoso commented Aug 17, 2021

Uh oh!

amartini51 commented Aug 19, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amartini51 commented Aug 23, 2021

Uh oh!

swift-ci commented Aug 23, 2021

Uh oh!

amartini51 commented Aug 24, 2021

Uh oh!

Uh oh!

amartini51 Aug 10, 2021 •

edited

Loading

amartini51 Aug 17, 2021 •

edited

Loading