-
Notifications
You must be signed in to change notification settings - Fork 58
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
Send API returns a write-only channel #302
Conversation
This simplifies APIs by removing TrySend and SendCtx(), and allowing the caller of Send more flexability, More flexability is provided by a channel as it allows the caller to chose to use it in a select, range loop, etc. when writing a message.
// IsLocal returns true if the session is local. | ||
IsLocal() bool | ||
// Send returns the peer's outgoing message channel. | ||
Send() chan<- Message |
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 find naming Send
confusing. It was clear in previous meaning and value. But looking somewhere in the code at line peer.Send()
it is not clear that it returns a channel now. And as the signature is changed (thus making new design incompatible with old behaviour) - maybe we can give it better name? At least smth like SendCh
? What do you think?
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 reason I left it as Send
is because that pairs with the existing Recv
function that also returns a channel. If we change Send
to something else, then we should also change Recv
.
// Recv returns a channel of messages from the peer.
Recv() <-chan Message
// Send returns the peer's outgoing message channel.
Send() chan<- Message
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.
Yeah, I see. While we're making a refactoring and can give better naming for both, I'm ok to keep it as is. So it's up to you: if you agree - let's rename! If no - no problem.
Co-authored-by: Konstantin Burkalev <[email protected]>
Co-authored-by: Konstantin Burkalev <[email protected]>
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.
👍🏻 LGTM!
Description, Motivation and Context
This PR simplifies APIs by removing
TrySend
andSendCtx
, and allows the caller ofSend
more flexability. More flexibility is provided by a channel as it allows the caller to choose to use it in a select, range loop, etc. when writing a message.Addresses Issue #291
What is the current behavior?
A message is passed into the blocking
Send
or non-blockingTrySend
, or a message and context is passed intoSendCtx
.What is the new behavior?
The
Send
function returns a write-only message channel. TheTrySend
andSendCtx
functions disappear.What kind of change does this PR introduce?
This might not be considered a breaking change if
Send
is considered an internal API used when writing your own router or client implementation using thewamp
package. You can see this is the case since all of the examples do not need to change.Checklist: