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

implement support for ?Sync callbacks #264

Merged
merged 1 commit into from
Dec 20, 2022

Conversation

zetanumbers
Copy link
Contributor

Resolves #263

@codecov
Copy link

codecov bot commented Dec 15, 2022

Codecov Report

Merging #264 (5800990) into main (6acd555) will increase coverage by 0.50%.
The diff coverage is 83.33%.

❗ Current head 5800990 differs from pull request most recent head 2ffa136. Consider uploading reports for the commit 2ffa136 to get more accurate results

@@            Coverage Diff             @@
##             main     #264      +/-   ##
==========================================
+ Coverage   86.27%   86.77%   +0.50%     
==========================================
  Files          31       31              
  Lines        2478     2481       +3     
==========================================
+ Hits         2138     2153      +15     
+ Misses        340      328      -12     
Impacted Files Coverage Δ
socketio/src/client/callback.rs 43.75% <ø> (+6.25%) ⬆️
socketio/src/client/client.rs 94.91% <ø> (ø)
socketio/src/client/raw_client.rs 77.63% <66.66%> (ø)
socketio/src/client/builder.rs 92.95% <100.00%> (ø)
engineio/src/asynchronous/client/async_client.rs 96.27% <0.00%> (+0.67%) ⬆️
engineio/src/asynchronous/client/builder.rs 91.93% <0.00%> (+0.80%) ⬆️
engineio/src/asynchronous/async_socket.rs 71.28% <0.00%> (+0.99%) ⬆️
...asynchronous/async_transports/websocket_general.rs 80.00% <0.00%> (+1.81%) ⬆️
...eio/src/asynchronous/async_transports/websocket.rs 91.89% <0.00%> (+2.70%) ⬆️
.../asynchronous/async_transports/websocket_secure.rs 90.78% <0.00%> (+2.78%) ⬆️
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Owner

@1c3t3a 1c3t3a left a comment

Choose a reason for hiding this comment

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

LGTM % comments. Thanks for the patch :)

socketio/src/client/builder.rs Show resolved Hide resolved
socketio/src/client/builder.rs Show resolved Hide resolved
@1c3t3a
Copy link
Owner

1c3t3a commented Dec 20, 2022

I am sorry, I missunderstood the mut thing here, could you drop commit 5800990? Then we just merge the first one.

@zetanumbers
Copy link
Contributor Author

Why drop 5800990?

@1c3t3a
Copy link
Owner

1c3t3a commented Dec 20, 2022

Why drop 5800990?

Currently the builder method doesn't need the mut, but it would be possible (and tbh also expected) that the builder needs mut here. If we were not putting mut here, adding it would be a breaking change, so we might as well just have it there for the case that we ever need mut in a future version of the interface. I didn't realize that acquiring the lock and putting something there doesn't need mut as it uses interior mutability. So I thought we could just drop the allow(unused_mut) comment which was wrong.

@1c3t3a 1c3t3a merged commit 295510f into 1c3t3a:main Dec 20, 2022
@zetanumbers
Copy link
Contributor Author

zetanumbers commented Dec 20, 2022

If we were not putting mut here, adding it would be a breaking change

It's nothing like that. This mut isn't even a part of the signature (check the docs.rs), but a part of the definition. That's because you can mutate immutable value by moving it out of the original immutable variable to the mutable one and mutate it however you want.

let a = "hello".to_string();
let mut b = a;
b.push_str(" world");

@zetanumbers zetanumbers deleted the unsync-callbacks branch December 20, 2022 14:01
@1c3t3a
Copy link
Owner

1c3t3a commented Dec 20, 2022

Mhh fair point because we don't take &self here but a moved value. Then it make sense to remove it actually.

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.

ClientBuilder::on(_any) should be able to take !Sync functions too
2 participants