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

Add support for WebSockets #3

Merged
merged 2 commits into from
Mar 3, 2020
Merged

Add support for WebSockets #3

merged 2 commits into from
Mar 3, 2020

Conversation

mullermp
Copy link
Contributor

@mullermp mullermp commented Feb 21, 2020

This change adds WebSocket modules (WS and WSS) to parse WebSocket URIs.

My implementation was inspired by HTTP and HTTPS modules/tests. If I am missing anything, please let me know.

Resources I used:
https://en.wikipedia.org/wiki/WebSocket
https://en.wikipedia.org/wiki/Uniform_Resource_Identifier#Syntax

Also looked at how Python does it:
https://websockets.readthedocs.io/en/stable/_modules/websockets/uri.html

Related AWS SDK For Ruby issue:
aws/aws-sdk-ruby#2232

RFC
https://tools.ietf.org/html/rfc6455#section-3
https://tools.ietf.org/html/rfc1738 (section 3.3 for "invalid URI with user info")

@mullermp
Copy link
Contributor Author

Ready for review @hsbt :) I notice there's also a PR from @ioquatix (#2) that's been out for a while, so this is just a friendly ping on these.

test/uri/test_ws.rb Show resolved Hide resolved
@ioquatix
Copy link
Member

I merged my own PR... does it simplify this implementation?

lib/uri/ws.rb Outdated Show resolved Hide resolved
lib/uri/wss.rb Outdated Show resolved Hide resolved
@mullermp
Copy link
Contributor Author

I merged my own PR... does it simplify this implementation?

Not really .. I saw you had an open PR sitting for a while with no approvals, so I figured I would be a nice gesture to get some attention on it. Didn't realize you had approval to merge it anyway X)

@mullermp mullermp requested a review from hsbt February 26, 2020 18:45
@ioquatix
Copy link
Member

Nice work updating the PR.

@mullermp
Copy link
Contributor Author

Let me know if there's anything else! This looks good to merge if the only comment was on documentation.

@mullermp
Copy link
Contributor Author

mullermp commented Mar 2, 2020

Friendly ping on this @hsbt @ioquatix. Is it all good to be merged?

@hsbt hsbt merged commit 98f4ce0 into ruby:master Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants