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 WebSocketClient::new_with_config to specify the WebSocket connection settings #975

Merged
merged 4 commits into from
Sep 16, 2021

Conversation

romac
Copy link
Member

@romac romac commented Sep 16, 2021

Closes: #974

This PR directly re-exports the tungstenite::protocol::WebSocketConfig. We could also define our own config type and convert it into the former if that's deemed preferable.

  • Referenced an issue explaining the need for the change
  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Added entry in .changelog/

Copy link
Member

@thanethomson thanethomson left a comment

Choose a reason for hiding this comment

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

Looks great! Just a changelog entry and these minor tweaks and I think we're good to go here.

rpc/src/client/transport/websocket.rs Outdated Show resolved Hide resolved
rpc/src/client/transport/websocket.rs Outdated Show resolved Hide resolved
Co-authored-by: Thane Thomson <thane@informal.systems>
@romac romac changed the title Add WebSocket::new_with_config to specify the WebSocket connection settings Add WebSocketClient::new_with_config to specify the WebSocket connection settings Sep 16, 2021
@romac romac marked this pull request as ready for review September 16, 2021 20:21
@codecov-commenter
Copy link

Codecov Report

Merging #975 (11a0b75) into master (80559bd) will increase coverage by 0.0%.
The diff coverage is 66.2%.

❗ Current head 11a0b75 differs from pull request most recent head d617af7. Consider uploading reports for the commit d617af7 to get more accurate results
Impacted file tree graph

@@          Coverage Diff           @@
##           master    #975   +/-   ##
======================================
  Coverage    72.1%   72.2%           
======================================
  Files         203     204    +1     
  Lines       16577   16621   +44     
======================================
+ Hits        11961   12006   +45     
+ Misses       4616    4615    -1     
Impacted Files Coverage Δ
light-client/examples/light_client.rs 0.0% <0.0%> (ø)
rpc/src/client.rs 16.9% <ø> (ø)
rpc/src/client/bin/main.rs 0.6% <0.0%> (-0.1%) ⬇️
rpc/src/lib.rs 100.0% <ø> (ø)
rpc/src/endpoint/consensus_params.rs 40.0% <40.0%> (ø)
rpc/src/method.rs 29.5% <50.0%> (+<0.1%) ⬆️
rpc/src/client/transport/websocket.rs 63.8% <66.6%> (+0.9%) ⬆️
rpc/tests/kvstore_fixtures.rs 95.4% <100.0%> (+<0.1%) ⬆️
rpc/src/endpoint/validators.rs 20.0% <0.0%> (-30.0%) ⬇️
rpc/src/endpoint/net_info.rs 75.0% <0.0%> (-25.0%) ⬇️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 80559bd...d617af7. Read the comment docs.

@thanethomson thanethomson merged commit 086acc8 into master Sep 16, 2021
@thanethomson thanethomson deleted the romac/expose-ws-config branch September 16, 2021 22:39
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.

rpc: Allow configuring the underlying tungstenite WebSocket connection
3 participants