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

Rename read/write to send/receive #1603

Merged
merged 6 commits into from Dec 19, 2017
Merged

Rename read/write to send/receive #1603

merged 6 commits into from Dec 19, 2017

Conversation

@rsoeldner
Copy link
Contributor

@rsoeldner rsoeldner commented Dec 18, 2017

.. as discussed #1596

@aeons
aeons approved these changes Dec 18, 2017
Copy link
Member

@aeons aeons left a comment

We should probably have deprecated aliases for the old names.

@@ -40,13 +40,13 @@ package object websocket {
* @param status The status code to return to a client making a non-websocket HTTP request to this route
*/
def WS[F[_]](
read: Stream[F, WebSocketFrame],
write: Sink[F, WebSocketFrame],
send: Stream[F, WebSocketFrame],

This comment has been minimized.

@rsoeldner

rsoeldner Dec 18, 2017
Author Contributor

@aeons I'm unsure how to handle the deprecation here

This comment has been minimized.

@rossabaker

rossabaker Dec 18, 2017
Member

I don't think we can in this case. It'll break for people using named arguments, but at least the types are unique. I think it'll be okay.

This comment has been minimized.

@jmcardon

jmcardon Dec 19, 2017
Member

👍 to this as well. It should be a simple fix for users.

* @param read The read side of the Exchange represents the stream of messages that should be sent to the client
* @param write The write side of the Exchange is a sink to which the framework will push the websocket messages
* @param send The send side of the Exchange represents the stream of messages that should be sent to the client
* @param receive The receive side of the Exchange is a sink to which the framework will push the websocket messages

This comment has been minimized.

@cquiroz

cquiroz Dec 18, 2017
Member

Maybe the description of receive could be clearer on the line of receiving messages from the client

This comment has been minimized.

@rsoeldner

rsoeldner Dec 18, 2017
Author Contributor

Maybe change client to consumer (on the send parameter) and producer on the receive parameter ?

This comment has been minimized.

@jmcardon

jmcardon Dec 19, 2017
Member

Something like The incoming messages to this service works as well. In a sense, it's even easier to think about incoming and outgoing messages.

You could do that for the client as well. So you can actually say "The send side of the exchange represents the outgoing messages sent from this service."

This mostly following from @rossabaker 's incentive to make this agnostic to where its done, so it works with the client.

@cquiroz
Copy link
Member

@cquiroz cquiroz commented Dec 18, 2017

Looks good. I guess existing users will get a compilation error right away forcing them to change the code and it's a simple rename in this case

read: Stream[F, WebSocketFrame],
write: Sink[F, WebSocketFrame]
)
@deprecatedName('read) send: Stream[F, WebSocketFrame],

This comment has been minimized.

@ChristopherDavenport

ChristopherDavenport Dec 18, 2017
Member

Can the deprecation be versioned here?

@@ -40,13 +40,13 @@ package object websocket {
* @param status The status code to return to a client making a non-websocket HTTP request to this route
*/
def WS[F[_]](
read: Stream[F, WebSocketFrame],
write: Sink[F, WebSocketFrame],
send: Stream[F, WebSocketFrame],

This comment has been minimized.

@rossabaker

rossabaker Dec 18, 2017
Member

I don't think we can in this case. It'll break for people using named arguments, but at least the types are unique. I think it'll be okay.

write: Sink[F, WebSocketFrame]
)
@deprecatedName('read, "0.18-M6") send: Stream[F, WebSocketFrame],
@deprecatedName('write, "0.18-M6") receive: Sink[F, WebSocketFrame]

This comment has been minimized.

@rossabaker

rossabaker Dec 18, 2017
Member

Sorry, one last nitpick: the version that will include this is 0.18.0-M7.

This comment has been minimized.

@rsoeldner

rsoeldner Dec 18, 2017
Author Contributor

Whoop, right 👍 Thank You

@rsoeldner rsoeldner force-pushed the rsoeldner:ws-naming branch to a311b7c Dec 18, 2017
Copy link
Member

@rossabaker rossabaker left a comment

And after all that, @deprecatedName only takes one argument in Scala 2.11. Sorry about that. 😞

Copy link
Member

@jmcardon jmcardon left a comment

I think the receive description and the travis failure should be changed, but otherwise everything seems fine. Fix that and it's a green from me 👍

* @param read The read side of the Exchange represents the stream of messages that should be sent to the client
* @param write The write side of the Exchange is a sink to which the framework will push the websocket messages
* @param send The send side of the Exchange represents the stream of messages that should be sent to the client
* @param receive The receive side of the Exchange is a sink to which the framework will push the websocket messages

This comment has been minimized.

@jmcardon

jmcardon Dec 19, 2017
Member

Something like The incoming messages to this service works as well. In a sense, it's even easier to think about incoming and outgoing messages.

You could do that for the client as well. So you can actually say "The send side of the exchange represents the outgoing messages sent from this service."

This mostly following from @rossabaker 's incentive to make this agnostic to where its done, so it works with the client.

@@ -40,13 +40,13 @@ package object websocket {
* @param status The status code to return to a client making a non-websocket HTTP request to this route
*/
def WS[F[_]](
read: Stream[F, WebSocketFrame],
write: Sink[F, WebSocketFrame],
send: Stream[F, WebSocketFrame],

This comment has been minimized.

@jmcardon

jmcardon Dec 19, 2017
Member

👍 to this as well. It should be a simple fix for users.

rsoeldner added 2 commits Dec 19, 2017
@rsoeldner
Copy link
Contributor Author

@rsoeldner rsoeldner commented Dec 19, 2017

@rossabaker next try 🎉

@aeons
aeons approved these changes Dec 19, 2017
@rossabaker rossabaker merged commit 10e1293 into http4s:master Dec 19, 2017
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.