zeromq / zmq.rs

A native implementation of ØMQ in Rust
https://crates.io/crates/zeromq
MIT License
1.16k stars 100 forks source link

Breaking change with minor version increase #144

Closed Dav1d23 closed 3 years ago

Dav1d23 commented 3 years ago

Hello,

first of all, thanks for the crate!

I would like to point out that with version 0.2.1 a backward incompatibility has been created. While zeromq::ZmqMessage provided impl From<Vec<u8, Global>> for ZmqMessage in 0.2.0, it does not in 0.2.1.

As far as I know, this change requires a major version increase - which is in this case 0.3.0.

Not sure how this should be handles, though :)

Alexei-Kornienko commented 3 years ago

Could you please provide more details about what exactly is broken?

Alexei-Kornienko commented 3 years ago

Thanks, I'll take a look at this.

Dav1d23 commented 3 years ago

Sorry, finger slipped over "enter" :) Github UI does not forgive ;)

Alexei-Kornienko commented 3 years ago

Ah. You are right - impl From<Vec<u8>> was changed for impl From<Bytes>. Should be a trivial fix. So I'll just add it back and publish a new minor version

Alexei-Kornienko commented 3 years ago

Bumped a new version. Feel free to reopen in case of other issues

Dav1d23 commented 3 years ago

Hello, I have the feeling there is also the other way around missing

impl From<ZmqMessage> for Vec<u8> {
    fn from(m: ZmqMessage) -> Self {
        m.data.to_vec()
    }
}

Could you please check if that is the case?

(I've already updated my code, but I feel it would be cleaner to restore the public API :) )

Alexei-Kornienko commented 3 years ago

Hm.. the problem with this one is that it's indeed a breaking change... This part was changed when we've added the possibility to create multipart messages. So right now ZmqMessage might be a multipart message and in such case it cannot be converted to a plain Vec

Alexei-Kornienko commented 3 years ago

I can add a TryFrom impl. However implementing From is logically not correct

Dav1d23 commented 3 years ago

I assumed that since I have to build the BytesMut and fill them in with the parts (assuming I got it correctly :)).

So, right now I'm doing something like this

    let msg: zeromq::ZmqMessage = ...;
    let mut buf = BytesMut::with_capacity(1024);
    for val in msg.iter() {
        buf.put_slice(val.as_ref());
    }

Would you be able to build the Vec the same way? It should be "safe" to do?

Anyway, I just wanted to point our that maybe in this case it would be good to bump the major version, but of course up to you ;)

Alexei-Kornienko commented 3 years ago

It will be "safe" but I guess in most cases it will not be an expected behaviour. If you use multipart messages for whatever reason just concatenate them might not be the best idea... I guess I will add a TryFrom impl. And would actually bump a major version... I guess I should also yank 0.2.x releases in such case...

Dav1d23 commented 3 years ago

Sure, I admit I don't know zmq multipart messages - if they can come from different "connection" then for sure what I'm doing is wrong. But in my (simple) use case I guess it is ok :)

Anyway, thanks for the time you put keeping this crate clean!

Dav1d23 commented 3 years ago

Edit: while writing I realized what you meant. Indeed, concat may not be what you want, depending on what the sender was sending.

Alexei-Kornienko commented 3 years ago

You are correct in terms of "I've got all the pieces." cause zmq returns all parts or none. However it's not that simple on application side. Multipart messages can contain for example one or more address envelopes (used in router/dealer) socket. And it may also be used in application level if user wants to provide his own routing. So user can in theory read and send multipart messages.

Alexei-Kornienko commented 3 years ago

Release version 0.3.0 and yank 0.2.1 and 0.2.2. Let me know if you have any issues