Skip to content
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

Add some docs to note on which message types a bootstrapped Channel w… #207

Merged
merged 3 commits into from
Mar 21, 2018

Conversation

normanmaurer
Copy link
Member

…ill operate.

Motivation:

It's not clear to a user which inbound / outbound messages is expected for the different Channel implementations. This can result into wrong code and confused users.

Modifications:

Add some docs to explain which messages are supported / expected by the different Channels that can be bootstrapped.

Result:

More clear docs.

@normanmaurer normanmaurer requested review from weissi and Lukasa March 21, 2018 07:49
@normanmaurer normanmaurer self-assigned this Mar 21, 2018
@normanmaurer normanmaurer added this to the 1.3.0 milestone Mar 21, 2018
@normanmaurer normanmaurer added the 🔨 semver/patch No public API change. label Mar 21, 2018
Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Just a few nits here.

/// The returned `ServerSocketChannel` by `bind` is the server channel and will fire the accepted `SocketChannel`s through the `ChannelPipeline` via `fireChannelRead`
/// and so operate on `Channel` as inbound messages. Outbound messages are not supported by `ServerSocketChannel`, which means each write attempt will be failed.
///
/// The accepted `SocketChannel` will operate on `ByteBuffer` as inbound and `IOData` as outbound messages.
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's change this to: "The EventLoopFuture returned by bind will fire with a ServerSocketChannel. This is the channel that owns the listening socket. Each time it accepts a new connection it will fire a SocketChannel through the ChannelPipeline via fireChannelRead: as a result, the ServerSocketChannel operates on SocketChannels as inbound messages. Outbound messages are not supported on a ServerSocketChannel which means that each write attempt will fail.

Accepted SocketChannels operate on ByteBuffer as inbound data, and IOData as outbound data."

Copy link
Member Author

Choose a reason for hiding this comment

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

@Lukasa I think we should still say: as a result, the ServerSocketChannel operates on Channels as inbound messages...

SocketChannel is not public so it may be confusing otherwise if the user tries to use it as InboundIn and can't. WDYT ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, that's fine.

@@ -410,6 +421,7 @@ public final class ClientBootstrap {
/// try channel.closeFuture.wait() // Wait until the channel un-binds.
/// ```
///
/// The `DatagramChannel` will operate on `AddressEnevelope<ByteBuffer>` as inbound and outbound messages.
Copy link
Contributor

Choose a reason for hiding this comment

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

AddressedEnvelope.

@normanmaurer normanmaurer force-pushed the channel_message_docs branch from 971d0c6 to 04b8003 Compare March 21, 2018 13:10
@normanmaurer
Copy link
Member Author

@Lukasa PTAL again

/// the `ServerSocketChannel` operates on `Channel`s as inbound messages. Outbound messages are not supported on a `ServerSocketChannel`
/// which means that each write attempt will fail.
///
/// Accepted `SocketChannel`s operate on `ByteBuffer` as inbound data, and IOData as outbound data.
Copy link
Contributor

Choose a reason for hiding this comment

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

Backticks around IOData.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

…ill operate.

Motivation:

It's not clear to a user which inbound / outbound messages is expected for the different Channel implementations. This can result into wrong code and confused users.

Modifications:

Add some docs to explain which messages are supported / expected by the different Channels that can be bootstrapped.

Result:

More clear docs.
@normanmaurer normanmaurer force-pushed the channel_message_docs branch from a33dda4 to d2f19f2 Compare March 21, 2018 14:19
Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Please rebase, then merge when the tests go green.

@normanmaurer normanmaurer merged commit 3c8caab into apple:master Mar 21, 2018
@normanmaurer normanmaurer deleted the channel_message_docs branch March 21, 2018 14:44
weissi pushed a commit to weissi/swift-nio that referenced this pull request Jun 13, 2020
weissi pushed a commit to weissi/swift-nio that referenced this pull request Feb 3, 2024
Motivation:

Now that Swift 5.9 is GM we should update the supported versions and
remove 5.6

Modifications:

* Update `Package.swift`
* Remove `#if swift(>=5.7)` guards
* Delete the 5.6 docker compose file and make a 5.10 one
* Update docs

Result:

Remove support for Swift 5.6, add 5.10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants