-
Notifications
You must be signed in to change notification settings - Fork 65
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
Make Sink
and future-util
optional
#143
Make Sink
and future-util
optional
#143
Conversation
…async-tungstenite into BillGoldenWater-feat/simple_send_method
…sink-and-future-util
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks mostly good, thanks!
In addition to these comments, can you also clean up the commit history? Just push --force
to this PR afterwards.
#[cfg(not(feature = "sink"))] | ||
impl<S> WebSocketStream<S> { | ||
/// Simple send method to replace `futures_sink::Sink` (till v0.3). | ||
pub async fn send(&mut self, msg: Message) -> Result<(), WsError> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't hurt to provide this always, and should make sure that both this and the Sink
impl are covered by the tests and running on the CI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think apart from this one it's all ready
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new function is already used by everything (e.g. the autobahn tests). Maybe you can change it so that the server uses Sink::send
and the client the new function (together with a comment why this is done)
Co-authored-by: Sebastian Dröge <[email protected]>
I prefer to keep the full history in my repo, even if merging my partner's code goes messy due to less experience. You can choose to squash and merge to avoid bringing those messy commits into the mainline. |
btw If a new and better |
That sounds like a good idea |
I've put a version with cleaned up history here: #144 Let me know if you want to change the commit message. |
Merged as part of #144 |
fixs #142