Skip to content

async -> enqueue. await -> enqueueAndWait #12

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 14, 2023
Merged

Conversation

dfed
Copy link
Owner

@dfed dfed commented Feb 12, 2023

Resolves #10

@dfed dfed force-pushed the dfed--async-await-rename branch from 2b8f252 to 3454ea6 Compare February 12, 2023 23:33
@codecov
Copy link

codecov bot commented Feb 12, 2023

Codecov Report

Merging #12 (3454ea6) into main (8b6a898) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main       #12   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            7         7           
  Lines          752       752           
=========================================
  Hits           752       752           
Impacted Files Coverage Δ
Sources/AsyncQueue/ActorQueue.swift 100.00% <100.00%> (ø)
Sources/AsyncQueue/FIFOQueue.swift 100.00% <100.00%> (ø)
Tests/AsyncQueueTests/ActorQueueTests.swift 100.00% <100.00%> (ø)
Tests/AsyncQueueTests/FIFOQueueTests.swift 100.00% <100.00%> (ø)

@dfed dfed requested review from bachand and Gabardone February 12, 2023 23:41
@dfed dfed marked this pull request as ready for review February 12, 2023 23:41
Copy link
Collaborator

@Gabardone Gabardone left a comment

Choose a reason for hiding this comment

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

I like this naming better. The prior version had too much baggage from the other APIs that use the same names.

@dfed dfed merged commit 1ecda35 into main Feb 14, 2023
@dfed dfed deleted the dfed--async-await-rename branch February 14, 2023 06:30
Copy link
Collaborator

@bachand bachand left a comment

Choose a reason for hiding this comment

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

Nice. I agree it's good to pick unique names that don't overlap with Swift language names.

@@ -78,7 +78,7 @@ public final class ActorQueue<ActorType: Actor> {

// MARK: Public

/// Sets the actor context within which each `async` and `await`ed task will execute.
/// Sets the actor context within which each `enqueue` and `enqueueAndWait` task will execute.
Copy link
Collaborator

Choose a reason for hiding this comment

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

"enqueue and enqueueAndWait" -> "enqueueed and enqueueAndWaited"?

@@ -54,7 +54,7 @@ public final class FIFOQueue: Sendable {
/// Schedules an asynchronous task for execution and immediately returns.
/// The scheduled task will not execute until all prior tasks – including suspended tasks – have completed.
/// - Parameter task: The task to enqueue.
public func async(_ task: @escaping @Sendable () async -> Void) {
public func enqueue(_ task: @escaping @Sendable () async -> Void) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to edit this await in this line?

/// Asynchronous tasks sent to this queue work as they would in a `DispatchQueue` type. Attempting to `await` this queue from a task executing on this queue will result in a deadlock.

Copy link
Owner Author

Choose a reason for hiding this comment

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

oooooh solid find!

@@ -36,46 +36,46 @@ final class FIFOQueueTests: XCTestCase {

// MARK: Behavior Tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

There are at least a couple places in the file like this that we should clean up.

// You can prove this to yourself by replacing `systemUnderTest.async` above with `Task`.

dfed added a commit that referenced this pull request Feb 21, 2023
@dfed
Copy link
Owner Author

dfed commented Feb 21, 2023

Pushed feedback up into #13

dfed added a commit that referenced this pull request Feb 21, 2023
#13)

* Make FIFOQueue, ActorQueue, and tests pass strict concurrency checking

* Post-merge review feedback from #9 & #12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename async and await on both FIFOQueue and ActorQueue
3 participants