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

Passing Headers to WebSocketClient.connect() #1282

Closed
wants to merge 0 commits into from
Closed

Passing Headers to WebSocketClient.connect() #1282

wants to merge 0 commits into from

Conversation

ruthvik125
Copy link
Contributor

Pull Request

For issue #1236
-> Passing a hyper::Request to which headers can be inserted ,instead of a url ,in connect_async()

Type Of Change

  • New feature (non-breaking change which adds functionality)

@CLAassistant
Copy link

CLAassistant commented Oct 18, 2023

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ cjdsellers
✅ ruthvik125
❌ ruthvik


ruthvik seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@ruthvik125
Copy link
Contributor Author

We can use a hyper::Request to which we can insert headers and pass it in connect_async() to connect from the client. Is adding an extra parameter for headers: for every function that is calling connect() or connect_with_server() better , or is making a separate function for connecting with headers better?

@cjdsellers
Copy link
Member

We can use a hyper::Request to which we can insert headers and pass it in connect_async() to connect from the client. Is adding an extra parameter for headers: for every function that is calling connect() or connect_with_server() better , or is making a separate function for connecting with headers better?

Hi @ruthvik125
Thanks for the contribution here!
I would say an additional parameter with a None like default would be the best way to go.

@@ -359,6 +364,7 @@ impl WebSocketClient {
#[staticmethod]
fn connect(
url: String,
headers:Vec<(&str,&str)>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

You'll need to take a Vec<(String, String)> because having lifetimes will be problematic in the Python API. And you want to store these headers in the WebsocketInnerClient so that they can be re-used in case of a reconnect.

@@ -86,7 +86,11 @@ impl WebSocketClientInner {
/// Connects with the server creating a tokio-tungstenite websocket stream.
#[inline]
pub async fn connect_with_server(url: &str) -> Result<(MessageWriter, MessageReader), Error> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You want to pass a clone of the stored Vec<(String, String)> here. Then consume and add the key value pairs to the request.

@cjdsellers
Copy link
Member

We also need to run the pre-commit checks:

make pre-commit

or

pre-commit run --all-files

Many thanks!

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.

4 participants