Add a way to get a handler from a pipeline #974

Palleas · 2019-04-18T04:36:43Z

Motivation:

#966

Modifications:

Add a handler(type:) method on ChannelPipeline
Add a test in ChannelPipelineTest

Result:

Provides a nicer way to get a typed handler directly from the pipeline

Closes #966

swift-nio-bot · 2019-04-18T04:36:45Z

Can one of the admins verify this patch?

swift-nio-bot · 2019-04-18T04:36:45Z

Can one of the admins verify this patch?

swift-nio-bot · 2019-04-18T04:36:45Z

Can one of the admins verify this patch?

swift-nio-bot · 2019-04-18T04:36:45Z

Can one of the admins verify this patch?

swift-nio-bot · 2019-04-18T04:36:46Z

Can one of the admins verify this patch?

Lukasa · 2019-04-18T08:04:26Z

Sources/NIO/ChannelPipeline.swift

+    ///
+    /// - parameters:
+    ///     - handlerType: the type of `ChannelHandler` to returns
+    public func handler<T: ChannelHandler>(type _: T.Type) -> EventLoopFuture<T> {


Should we have a version of this for each of our context methods? I've always thought the context methods were pretty weird, and ideally I'd get rid of them and replace them with these versions. @normanmaurer is there a good reason not to do that?

@Lukasa I think the context ones are there so you can search and later on remove without walking the pipeline. But I agree they're a bit weird, not sure if we should kill them though.

weissi · 2019-04-18T09:58:00Z

Tests/NIOTests/ChannelPipelineTest.swift

+        XCTAssertTrue(result2 === handler2)
+
+        let result3 = try channel.pipeline.handler(type: SimpleTypedHandler3.self).wait()
+        XCTAssertTrue(result3 === handler3)


nice! We could do some more tests here, like

two handlers of the same type in the pipeline, verify that we get the first of those

no handler of the given type there, verify that we get nil

weissi · 2019-04-18T09:58:31Z

Sources/NIO/ChannelPipeline.swift

@@ -1555,6 +1555,20 @@ extension ChannelPipeline: CustomDebugStringConvertible {
        return desc.joined(separator: "\n")
    }

+    /// Returns the `ChannelHandlerContext` that belongs to a `ChannelHandler` of the given type.


that documentation isn't quite right. It returns the first ChannelHandler of a given type.

weissi · 2019-04-18T09:58:49Z

Sources/NIO/ChannelPipeline.swift

+    ///
+    /// - parameters:
+    ///     - handlerType: the type of `ChannelHandler` to returns
+    public func handler<T: ChannelHandler>(type _: T.Type) -> EventLoopFuture<T> {


I think we should name the generic parameter Handler instead of just T

weissi

Thanks, that looks great already. A few suggestions

weissi

Awesome, just two nits left

Sources/NIO/ChannelPipeline.swift

weissi

Thank you! Looks good to me

weissi · 2019-04-18T15:50:33Z

@swift-nio-bot test this please

Lukasa · 2019-04-30T12:16:15Z

Sources/NIO/ChannelPipeline.swift

+    /// - parameters:
+    ///     - handlerType: the type of `ChannelHandler` to return.
+    public func handler<Handler: ChannelHandler>(type _: Handler.Type) -> EventLoopFuture<Handler> {
+        return context(handlerType: Handler.self).map { context in


Nit: can we use the explicit self keyword here?

@Lukasa done!

Motivation: apple#966 Modifications: - Add a `handler(type:)` method on ChannelPipeline - Add a test in ChannelPipelineTest Result: Provides a nicer way to get a typed handler directly from the pipeline Closes apple#966

Lukasa · 2019-04-30T13:56:20Z

@swift-nio-bot test this please

weissi · 2019-04-30T14:12:58Z

@Mordil You'll like this PR I think ;). @MrMage & @glbrntt I think you also have code like this around that should now be simpler (getting the http2 multiplexer from a client connection).

Lukasa requested review from normanmaurer and weissi April 18, 2019 08:02

Lukasa added the semver/minor Adds new public API. label Apr 18, 2019

Lukasa added this to the 2.1.0 milestone Apr 18, 2019

Lukasa reviewed Apr 18, 2019

View reviewed changes

weissi reviewed Apr 18, 2019

View reviewed changes

weissi requested changes Apr 18, 2019

View reviewed changes

Palleas force-pushed the romain/966 branch from 3af2eb6 to 4d0bde7 Compare April 18, 2019 15:40

weissi requested changes Apr 18, 2019

View reviewed changes

Sources/NIO/ChannelPipeline.swift Outdated Show resolved Hide resolved

Sources/NIO/ChannelPipeline.swift Outdated Show resolved Hide resolved

weissi reviewed Apr 18, 2019

View reviewed changes

Sources/NIO/ChannelPipeline.swift Outdated Show resolved Hide resolved

Palleas force-pushed the romain/966 branch from 56957d5 to 07df85f Compare April 18, 2019 15:49

weissi approved these changes Apr 18, 2019

View reviewed changes

glbrntt mentioned this pull request Apr 23, 2019

Verify TLS/ALPN were successful before returning a connection grpc/grpc-swift#443

Merged

weissi requested a review from Lukasa April 30, 2019 11:05

Lukasa requested changes Apr 30, 2019

View reviewed changes

Palleas force-pushed the romain/966 branch from 07df85f to 9188c6b Compare April 30, 2019 13:52

Add a way to get a handler from a pipeline

530a4e4

Motivation: apple#966 Modifications: - Add a `handler(type:)` method on ChannelPipeline - Add a test in ChannelPipelineTest Result: Provides a nicer way to get a typed handler directly from the pipeline Closes apple#966

Palleas force-pushed the romain/966 branch from 9188c6b to 530a4e4 Compare April 30, 2019 13:52

Lukasa approved these changes Apr 30, 2019

View reviewed changes

weissi merged commit 81538ec into apple:master Apr 30, 2019

Palleas deleted the romain/966 branch April 30, 2019 15:09

weissi mentioned this pull request May 10, 2019

retrieving the http/2 multiplexer swift-server-community/APNSwift#20

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a way to get a handler from a pipeline #974

Add a way to get a handler from a pipeline #974

Palleas commented Apr 18, 2019

swift-nio-bot commented Apr 18, 2019

swift-nio-bot commented Apr 18, 2019

swift-nio-bot commented Apr 18, 2019

swift-nio-bot commented Apr 18, 2019

swift-nio-bot commented Apr 18, 2019

Lukasa Apr 18, 2019

weissi Apr 18, 2019

weissi Apr 18, 2019

weissi Apr 18, 2019

weissi Apr 18, 2019

weissi left a comment

weissi left a comment

weissi left a comment

weissi commented Apr 18, 2019

Lukasa Apr 30, 2019

Palleas Apr 30, 2019

Lukasa commented Apr 30, 2019

weissi commented Apr 30, 2019 •

edited

Loading

Add a way to get a handler from a pipeline #974

Add a way to get a handler from a pipeline #974

Conversation

Palleas commented Apr 18, 2019

swift-nio-bot commented Apr 18, 2019

swift-nio-bot commented Apr 18, 2019

swift-nio-bot commented Apr 18, 2019

swift-nio-bot commented Apr 18, 2019

swift-nio-bot commented Apr 18, 2019

Lukasa Apr 18, 2019

Choose a reason for hiding this comment

weissi Apr 18, 2019

Choose a reason for hiding this comment

weissi Apr 18, 2019

Choose a reason for hiding this comment

weissi Apr 18, 2019

Choose a reason for hiding this comment

weissi Apr 18, 2019

Choose a reason for hiding this comment

weissi left a comment

Choose a reason for hiding this comment

weissi left a comment

Choose a reason for hiding this comment

weissi left a comment

Choose a reason for hiding this comment

weissi commented Apr 18, 2019

Lukasa Apr 30, 2019

Choose a reason for hiding this comment

Palleas Apr 30, 2019

Choose a reason for hiding this comment

Lukasa commented Apr 30, 2019

weissi commented Apr 30, 2019 • edited Loading

weissi commented Apr 30, 2019 •

edited

Loading