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

Fix request and response translation #51

Merged
merged 3 commits into from
Jul 3, 2024

Conversation

guoye-zhang
Copy link
Contributor

Motivation

Fix the translation between Foundation types and HTTP types.

Modifications

The code is mostly borrowed from swift-http-types:

https://github.com/apple/swift-http-types/tree/main/Sources/HTTPTypesFoundation

Translates the string encoding (URLSession expects ISOLatin1) and handles multi-value fields.

Result

Should address the cookie issue among other issues.

Test Plan

All existing tests still pass.

@simonjbeaumont
Copy link
Collaborator

Change looks great, thank you @guoye-zhang!

Do you think we can extend the tests to cover the different delimiter appending logic based?

@guoye-zhang
Copy link
Contributor Author

I updated the test with multiple values. ; joining is already tested by the cookie header.

Copy link
Collaborator

@simonjbeaumont simonjbeaumont left a comment

Choose a reason for hiding this comment

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

Thanks @guoye-zhang!

@czechboy0
Copy link
Collaborator

@swift-server-bot test this please

@czechboy0 czechboy0 enabled auto-merge (squash) July 3, 2024 07:29
@simonjbeaumont
Copy link
Collaborator

@swift-server-bot add to allowlist

@czechboy0
Copy link
Collaborator

The failed CI is a transitive infra issue, rekicking.

@czechboy0
Copy link
Collaborator

@swift-server-bot test this please

@czechboy0 czechboy0 merged commit 50e1f4a into apple:main Jul 3, 2024
5 checks passed
@czechboy0 czechboy0 added the semver/patch No public API change. label Jul 3, 2024
@guoye-zhang guoye-zhang deleted the translation branch November 2, 2024 14:44
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.

3 participants