Skip to content

Make completion-based send/receive functions public in URLSessionWebSocketTask #5030

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

Merged
merged 1 commit into from
Jul 31, 2024

Conversation

lxbndr
Copy link
Contributor

@lxbndr lxbndr commented Jul 26, 2024

Completion-based send and receive are public in Apple Foundation.

Perhaps, availability should be added to the containing type, but I added it here for consistency with async counterparts.

@parkera parkera requested a review from jrflat July 26, 2024 16:38
@parkera
Copy link
Contributor

parkera commented Jul 26, 2024

It would be a good idea to add a test case that calls into these (if we don't have one already).

@lxbndr lxbndr marked this pull request as draft July 26, 2024 22:59
@lxbndr lxbndr force-pushed the websocket-public-completion branch from 992a6c2 to b6b8475 Compare July 27, 2024 16:01
@lxbndr lxbndr marked this pull request as ready for review July 27, 2024 16:02
@lxbndr
Copy link
Contributor Author

lxbndr commented Jul 27, 2024

Test added. The test is the same as existing async-based test_webSocket, but ported to use completion handlers.

@jrflat
Copy link
Contributor

jrflat commented Jul 28, 2024

@swift-ci please test

1 similar comment
@jrflat
Copy link
Contributor

jrflat commented Jul 29, 2024

@swift-ci please test

@lxbndr
Copy link
Contributor Author

lxbndr commented Jul 29, 2024

Windows CI failed on checkout:

C:\Users\swift-ci\jenkins\workspace\swift-corelibs-foundation-PR-windows\swift-docc failed (ret=1): ['git', 'rebase', 'FETCH_HEAD']
b'error: cannot rebase: You have unstaged changes.\nerror: Please commit or stash them.\n'

At least Linux is green. But I am not sure if it actually runs these tests. I don't see any output related to URLSession tests.

@jrflat
Copy link
Contributor

jrflat commented Jul 30, 2024

LGTM thanks! Since web sockets are an experimental feature in curl, we'll need to run it with a curl where they're enabled to test it. I'll test it on a Fedora40 container to confirm

@lxbndr
Copy link
Contributor Author

lxbndr commented Jul 30, 2024

Just a small note: these API is what is actually used by async variants, which are already public. So I'd say it is pretty safe to open up a little more.

Copy link
Contributor

@jrflat jrflat left a comment

Choose a reason for hiding this comment

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

Yeah that's true, the tests are also all green for me on a container with web sockets enabled. Looks good and let's merge after Windows passes (I think that issue's been resolved). Thanks again!

@jrflat
Copy link
Contributor

jrflat commented Jul 30, 2024

@swift-ci please test Windows platform

@jrflat jrflat merged commit 8e4ddae into swiftlang:main Jul 31, 2024
2 of 3 checks passed
@lxbndr lxbndr deleted the websocket-public-completion branch July 31, 2024 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants