Skip to content

Conversation

clintonpi
Copy link
Contributor

Motivation:

To create a connected pair of ServerTransport and ClientTransport, users need to call InProcessTransport.makePair(serviceConfig:) which returns a tuple. The spelling of this API can be more intuitive and simple.

Modifications:

  • Restructure InProcessTransport to include two properties: server (the ServerTransport) and client (the ClientTransport), and an init(serviceConfig:) that pairs these properties.
  • Add missing GRPCInProcessTransport dependency to GRPCCore test target.

Result:

The spelling of the InProcessTransport API will be better.

Motivation:

To create a connected pair of `ServerTransport` and `ClientTransport`, users need to call `InProcessTransport.makePair(serviceConfig:)` which returns a tuple. The spelling of this API can be more intuitive and simple.

Modifications:

- Restructure `InProcessTransport` to include two properties: `server` (the `ServerTransport`) and `client` (the `ClientTransport`), and an `init(serviceConfig:)` that pairs these properties.
- Add missing GRPCInProcessTransport dependency to GRPCCore test target.

Result:

The spelling of the `InProcessTransport` API will be better.
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.

Mostly looks fine, left a few comments which need resolving.

Comment on lines 68 to 69
/// let inProcessServerTransport = InProcessTransport.Server()
/// let inProcessClientTransport = InProcessTransport.Client(serverTransport: inProcessServerTransport)
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 was wrong before your changes: can you update it?

/// ```swift
/// // Create and an in-process transport.
/// let inProcessTransport = InProcessServerTransport()
/// let inProcessTransport = InProcessTransport.Server()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, this is also wrong, can you update this one?

@@ -0,0 +1,359 @@
/*
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you rename this file? Normally when you extend a type you name the file <ThingBeingExtended>+<WhatItsExtendedWith>.swift so in this case InProcessTransport+Client.swift.

This makes things much easier to find: if you're looking for the in-process client transport then the obvious thing to search for is "InProcessTransport" because that's much more specific than "Client".

/// and a client using that server transport.
@available(macOS 15.0, iOS 18.0, watchOS 11.0, tvOS 18.0, visionOS 2.0, *)
public struct InProcessTransport: Sendable {
public let server = Self.Server()
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: initialize this in the init

Suggested change
public let server = Self.Server()
public let server: Self.Server

public let server = Self.Server()
public let client: Self.Client

/// Initializes a new ``InProcessTransport`` pairing a ``ServerTransport`` and a ``ClientTransport``.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// Initializes a new ``InProcessTransport`` pairing a ``ServerTransport`` and a ``ClientTransport``.
/// Initializes a new ``InProcessTransport`` pairing a ``Client`` and a ``Server``.

@@ -0,0 +1,83 @@
/*
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you rename this file to be InProcessTransport+Server.swift?

@glbrntt glbrntt added the ⚠️ semver/major Breaks existing public API. label Sep 24, 2024
@clintonpi clintonpi requested a review from glbrntt September 25, 2024 09:29
@glbrntt
Copy link
Collaborator

glbrntt commented Sep 25, 2024

Can you remove 60cc0bd? It's unrelated to the changes made in this PR so shouldn't be included here (it was fixed by #2073)

@glbrntt
Copy link
Collaborator

glbrntt commented Sep 25, 2024

Can you remove 60cc0bd? It's unrelated to the changes made in this PR so shouldn't be included here (it was fixed by #2073)

Nevermind, merging from main effectively undid this.

@glbrntt glbrntt merged commit 1fd34e7 into grpc:main Sep 25, 2024
5 of 8 checks passed
@rnro rnro mentioned this pull request Sep 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚠️ semver/major Breaks existing public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants