-
Notifications
You must be signed in to change notification settings - Fork 173
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
fix(client): adjust TransportSenderT #852
Conversation
This is trait contains `WebSocket` specific details and it's difficult to fix it properly with an extension trait in the current design. So this PR documents and marks it clearly that these methods are optional to implement, kind of ugly but better.
Co-authored-by: Alexandru Vasile <60601340+lexnv@users.noreply.github.com>
Co-authored-by: Alexandru Vasile <60601340+lexnv@users.noreply.github.com>
The
I suggest increasing the threshold to 80ms for this case 😄 |
Co-authored-by: James Wilson <james@jsdw.me>
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 like that the default impls don't fail now; it makes much more sense to have default impls when they can safely be ignored if the implementor doesn't care about that functionality :)
For that reason, I'm not sure they need the optional_
prefix; they are optional now in the same way that any trait fn with a default impl is, so I don't think the prefix adds anything.
Removed it now, I guess the documentation is clear enough now so we don't need that. |
This is trait contains
WebSocket
specific details and it's difficult to fix it properly with an extension traitin the current design.
So this PR documents and marks it clearly that these methods are optional to implement, kind of ugly but better.