Skip to content

Conversation

@simonjbeaumont
Copy link
Collaborator

This creates a new executable target, AsyncAwaitEcho, which is equivalent to Echo except that is makes use of the new async-await–based client and provider APIs.

Signed-off-by: Si Beaumont beaumont@apple.com

Copy link
Collaborator

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

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

Looks good aside from a few nits.

Could you also add a note to the README in Sources/Examples/Echo to mention that there are two versions, the original one and this new async/await one?

/// NOTE: This file should be removed when the `async` branch of `swift-argument-parser` has been
/// released: https://github.com/apple/swift-argument-parser/tree/async

#if compiler(>=5.5)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: && canImport(_Concurrency) and elsewhere

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in 4e909af.


@available(macOS 12, iOS 15, tvOS 15, watchOS 8, *)
func echoGet(client: Echo_EchoAsyncClient, message: String) async throws {
async let response = client.get(.with { $0.text = message })
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why async let here (and in collect)? Given we await on the next line it would be more idiomatic to await here instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, it was more to demonstrate that it's possible for this to be asynchronous. I.e. you could do other things after this line concurrently. But happy to change it. I guess anyone who sees let ... = await ... will know they could swap it for async let ... = ... and await later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in 377cd38.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This one hasn't been done for 'get'. A victim of the rebase?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops. Fixed in 3f5ee89.

@glbrntt glbrntt added the semver/none No version bump required. label Sep 28, 2021
@glbrntt glbrntt changed the base branch from async-await to 1.4.1-async-await September 28, 2021 08:26
@glbrntt glbrntt changed the base branch from 1.4.1-async-await to async-await September 28, 2021 08:27
Copy link
Collaborator

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

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

Could you rebase onto the 1.4.1-async-await branch as well?

Copy link
Collaborator Author

@simonjbeaumont simonjbeaumont left a comment

Choose a reason for hiding this comment

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

Addressed the inline comments.

Could you also add a note to the README in Sources/Examples/Echo to mention that there are two versions, the original one and this new async/await one?

Added in 800b72e.


@available(macOS 12, iOS 15, tvOS 15, watchOS 8, *)
func echoGet(client: Echo_EchoAsyncClient, message: String) async throws {
async let response = client.get(.with { $0.text = message })
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, it was more to demonstrate that it's possible for this to be asynchronous. I.e. you could do other things after this line concurrently. But happy to change it. I guess anyone who sees let ... = await ... will know they could swap it for async let ... = ... and await later.


@available(macOS 12, iOS 15, tvOS 15, watchOS 8, *)
func echoGet(client: Echo_EchoAsyncClient, message: String) async throws {
async let response = client.get(.with { $0.text = message })
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in 377cd38.

/// NOTE: This file should be removed when the `async` branch of `swift-argument-parser` has been
/// released: https://github.com/apple/swift-argument-parser/tree/async

#if compiler(>=5.5)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in 4e909af.

Signed-off-by: Si Beaumont <beaumont@apple.com>
Signed-off-by: Si Beaumont <beaumont@apple.com>
Signed-off-by: Si Beaumont <beaumont@apple.com>
Signed-off-by: Si Beaumont <beaumont@apple.com>
Signed-off-by: Si Beaumont <beaumont@apple.com>
@simonjbeaumont simonjbeaumont changed the base branch from async-await to 1.4.1-async-await September 29, 2021 11:05
@simonjbeaumont simonjbeaumont force-pushed the sb-async-echo-example-impl branch from 800b72e to a060723 Compare September 29, 2021 11:06
@simonjbeaumont
Copy link
Collaborator Author

@glbrntt rebased and retargeted PR for the 1.4.1 branch.

Signed-off-by: Si Beaumont <beaumont@apple.com>
@simonjbeaumont simonjbeaumont requested a review from glbrntt October 1, 2021 09:11
Copy link
Collaborator

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

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

Great, thanks Si!

@glbrntt glbrntt merged commit bbfd50b into grpc:1.4.1-async-await Oct 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

semver/none No version bump required.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants