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

add warp::ws2() as a more flexible websocket filter #34

Merged
merged 1 commit into from
Aug 9, 2018
Merged

Conversation

seanmonstar
Copy link
Owner

Motivated by #29

cc @caolan

@scrogson
Copy link

scrogson commented Aug 7, 2018

@seanmonstar What is the rationale behind the name Ws2 and ws2?

@seanmonstar
Copy link
Owner Author

@scrogson I'd expect them to replace the existing ws() filter, which returns a Ws. Changing them in 0.1 however is a breaking change, so I added a 2 suffix (similar to things like futures::Stream::concat2()). Do you think a different name is better?

@kellytk
Copy link
Contributor

kellytk commented Aug 7, 2018

@seanmonstar FWIW I've seen next and ng used in identifiers to indicate what you describe.

@scrogson
Copy link

scrogson commented Aug 8, 2018

I'd expect them to replace the existing ws() filter, which returns a Ws.

Does this mean that ws2 and Ws2 would become ws and Ws in v1.0?

Changing them in 0.1 however is a breaking change, so I added a 2 suffix (similar to things like futures::Stream::concat2()).

In my opinion (and I could be completely wrong or mean to suggest this, but...), anything < 1.0 is fair game for breakage. Given this is early days, I assume most people are still in the exploration phase and might not be upset about a small breaking change. I understand if you don't agree with me on that. 😉

Do you think a different name is better?

I guess my biggest complaint is seeing version numbers in the types rather than a module namespace.

Coming from the Erlang/Elixir world, it's not completely obvious if it's a versioning thing or the arity of the function. In Erlang the arity of a function is often used to document functions: module:function/arity due to the fact that functions can share the same name but take different arguments.

@seanmonstar
Copy link
Owner Author

Does this mean that ws2 and Ws2 would become ws and Ws in v1.0?

Yes, though rather, in 0.2.0.

In the Rust community (and the way cargo interprets semver), it's expected that upgrading a patch version of 0.x.y to 0.x.z will continue to compile. Cargo will eagerly download the newest patch version.

Coming from the Erlang/Elixir world, it's not completely obvious if it's a versioning thing or the arity of the function.

I've seen it far more often as "this is the newer version", with several examples in the futures crate. Though, it also happens to have a couple examples of including arity in the name, like join3, join4, etc... Yet even those are kind of lies, since it's not the actual arity (fut.join3(one, two)).

I've also occasionally seen ng (for next generation), like there was std::vec_ng for a while before Rust 1.0...

@caolan
Copy link

caolan commented Aug 8, 2018

Thanks @seanmonstar, this API will do what I need.

@scrogson
Copy link

scrogson commented Aug 8, 2018

I've seen it far more often as "this is the newer version", with several examples in the futures crate.

Good to know, thanks for taking the time to explain 👍

@jnicholls
Copy link
Contributor

You could always go straight to 0.2.0 now, rather than getting people to switch to a ws2() and then back to ws() later. You can go to 0.13483024824.0, it doesn't matter. Make the API the way you want before 1.0.

@kellytk
Copy link
Contributor

kellytk commented Aug 8, 2018

@jnicholls FWIW @seanmonstar described his rationale in #14 (comment) as "I'll probably hold off on a breaking change for at least a little bit, so that these papercuts can be grouped up."

@jnicholls
Copy link
Contributor

@kellytk Indeed, fair enough! :) Thanks.

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.

5 participants