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

Make HTTP type conversion code public #216

Merged
merged 3 commits into from Feb 12, 2024

Conversation

adam-fowler
Copy link
Contributor

Make NIOHTTP1 -> HTTPTypes and vice versa conversion code public

Motivation:

WebSocket APIs still use NIOHTTP1 types which means any library that wants to use the new types has to write a conversion step. Also OpenAPI runtime uses new HTTP types while both the server transports that are published use old NIOHTTP1 types. It is probably better that there is one place where this conversion code lives.

@@ -15,14 +15,14 @@
import HTTPTypes
import NIOHTTP1

private enum HTTP1TypeConversionError: Error {
public enum HTTP1TypeConversionError: Error {
Copy link
Contributor

Choose a reason for hiding this comment

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

We have to do our usual struct wrapping enum dance here otherwise we can never evolve this enum in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.
I was wondering why NIO never implemented a func ~ for these errors so switch and catch pattern matching was simplified. Currently to catch one of these "struct" errors you need

} catch let e as HTTP1TypeConversionError where e == .invalidStatusCode {

Which is a mouthful, and something I get wrong every time.
If this was added to allow pattern matching

    public static func ~= (_ lhs: Self, _ rhs: Error) -> Bool {
        guard let rhs = rhs as? Self else { return false }
        return lhs == rhs
    }

The catch statement gets reduced to

} catch HTTP1TypeConversionError.invalidStatusCode {

as if it is an enum.

Copy link
Contributor

Choose a reason for hiding this comment

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

In general, we try to stay away from custom operators or overloading more operators since they have quite a heavy impact on the compiler's performance. What we really want is extensible enums in non-library evolution mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What we really want is extensible enums in non-library evolution mode.

Been waiting for that one for a while.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@FranzBusch Is there anything holding merging this now?

Copy link
Contributor

@FranzBusch FranzBusch left a comment

Choose a reason for hiding this comment

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

LGTM!

@FranzBusch
Copy link
Contributor

@swift-server-bot test this please

@adam-fowler
Copy link
Contributor Author

Is this ready to merge? The API breakage CI seems stuck.

@FranzBusch
Copy link
Contributor

@swift-server-bot test this please

@FranzBusch
Copy link
Contributor

@yim-lee for some reason the API breakage pipeline is not starting can you check what's going wrong here?

@yim-lee
Copy link
Member

yim-lee commented Feb 9, 2024

@swift-server-bot add to allowlist

@yim-lee
Copy link
Member

yim-lee commented Feb 9, 2024

Just needed to add Adam to the allow list it seems. Should be ok now.

@adam-fowler
Copy link
Contributor Author

The API breakage is complaining about LineBasedFrameDecoder.decode(buffer:) has been removed

As I haven't touched this file I'm not sure why I'm getting this.

@FranzBusch FranzBusch enabled auto-merge (squash) February 12, 2024 10:22
@FranzBusch FranzBusch merged commit a3b640d into apple:main Feb 12, 2024
6 of 8 checks passed
@hamzahrmalik hamzahrmalik added the 🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0 label Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants