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

[API breaking] Add types to inbound/outbound debug handlers #81

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

[API breaking] Add types to inbound/outbound debug handlers #81

wants to merge 5 commits into from

Conversation

Davidde94
Copy link
Contributor

@Davidde94 Davidde94 commented Mar 2, 2020

Add generic types to inbound and outbound debug handlers. The current implementation is API-breaking, however it's entirely possible to implement in a "minor-version" way (detail below).

Motivation:

I've found that the provided debug handlers are not massively useful without types, as it's not possible (as far as I'm aware) to unwrap the data inside NIOAny outside of a channel handler. This means that to debug-print data inside my application I have to essentially recreate the logic provided by the print handlers. This seems redundant, and it would be nice to have an "official" implementation.

Modifications:

Add generic types T to both Inbound and DebugOutboundEventsHandler. Inside the read event, instead of passing down a NIOAny, pass the unwrapped data.

Result:

[After your change, what will change.]

Notes:

This is an API-breaking change. It's also possible to create new classes, e.g. TypedDebugInboundEventsHandler, etc. Alternatively we could cleverly expose something from NIOAny to allow unwrapping outside of a ChannelHandler. E.g. nioAny.unwrap(as: T).

@swift-server-bot
Copy link

Can one of the admins verify this patch?

1 similar comment
@swift-server-bot
Copy link

Can one of the admins verify this patch?

@Davidde94 Davidde94 requested review from weissi, glbrntt and Lukasa March 2, 2020 12:50
@Davidde94
Copy link
Contributor Author

@swift-server-bot test this please

@weissi weissi added the ⚠️ semver/major Breaks existing public API. label Mar 2, 2020
@weissi weissi added this to the 2.0.0 milestone Mar 2, 2020
@weissi
Copy link
Member

weissi commented Mar 2, 2020

Yeah, it should’ve been data(Any) not NIOAny... we can also make it fully typed but either solution is API breaking so will probably need to wait until NIO 3 (NIOExtras 2)

@weissi
Copy link
Member

weissi commented Mar 2, 2020

@swift-nio-bot add to whitelist

@weissi
Copy link
Member

weissi commented Mar 2, 2020

@swift-server-bot test this please

@Davidde94
Copy link
Contributor Author

@weissi as a short-term win, what do you think of implementing a similar but typed TypedDebugInboundPrintHandler for use now?

@glbrntt
Copy link
Contributor

glbrntt commented Mar 2, 2020

Are we explicitly opposed to having a public unwrap(as:) method for NIOAny?

@weissi
Copy link
Member

weissi commented Mar 3, 2020

Are we explicitly opposed to having a public unwrap(as:) method for NIOAny?

Yes. It’s deliberately opaque and allowing manual unwrapping doesn’t really enable anything that you couldn’t achieve today. Set your InboundIn to Any, then you have self.unwrapInboundIn to Any and from there to anywhere

@Davidde94
Copy link
Contributor Author

IMO something doesn't feel quite right about using Any, personally I would say fully-typed is generally better as it allows for less misuse

@weissi
Copy link
Member

weissi commented Mar 3, 2020

@Davidde94 this handler doesn’t have type information so typing it is impossible (without changing its type). It was designed to handle Any but by accident saved a NIOAny in the Event enum, we just missed this on review

@Lukasa Lukasa changed the base branch from master to main September 24, 2020 14:24
@Lukasa
Copy link
Contributor

Lukasa commented Sep 24, 2020

Updated this PR to target main.

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.

5 participants