Encoding and timestamp serialization remove from zenoh-ext #1520
Conversation
|
PR missing one of the required labels: {'internal', 'bug', 'breaking-change', 'new feature', 'enhancement', 'dependencies', 'documentation'} |
zenoh-ext/src/serialization.rs
Outdated
| @@ -608,12 +545,11 @@ mod tests { | |||
| fn timestamp_serialization() { | |||
There was a problem hiding this comment.
removed. Made it to make sure that we still can serialize timestamp and as an example, but yes, this is excessive
zenoh/src/api/encoding.rs
Outdated
| /// A UTF-8 string. | ||
| /// | ||
| /// Constant alias for string: `"zenoh/string"`. | ||
| /// | ||
| /// Usually used for types: `String`, `&str`, `Cow<str>`, `char`. | ||
| /// This encoding supposes that the payload was created with [`ZBytes::from::<String>`](crate::bytes::ZBytes::from) or similar | ||
| /// (`&str`, `Cow<str>`, `char`) and it's data can be acquired with [`ZBytes::try_to_string()`](crate::bytes::ZBytes::try_to_string) without an error. |
There was a problem hiding this comment.
There is no implementation for char. Btw, why enumerating similar here and not above for ZENOH_BYTES?
There was a problem hiding this comment.
removed char, added similar comment to bytes
zenoh/src/api/encoding.rs
Outdated
| /// A UTF-8 string. | ||
| /// | ||
| /// Constant alias for string: `"zenoh/string"`. | ||
| /// | ||
| /// Usually used for types: `String`, `&str`, `Cow<str>`, `char`. | ||
| /// This encoding supposes that the payload was created with [`ZBytes::from::<String>`](crate::bytes::ZBytes::from) or similar | ||
| /// (`&str`, `Cow<str>`, `char`) and it's data can be acquired with [`ZBytes::try_to_string()`](crate::bytes::ZBytes::try_to_string) without an error. |
There was a problem hiding this comment.
| /// (`&str`, `Cow<str>`, `char`) and it's data can be acquired with [`ZBytes::try_to_string()`](crate::bytes::ZBytes::try_to_string) without an error. | |
| /// (`&str`, `Cow<str>`, `char`) and its data can be accessed with [`ZBytes::try_to_string()`](crate::bytes::ZBytes::try_to_string). |
zenoh/src/api/encoding.rs
Outdated
| 14u16 => "zenoh/string", | ||
| 15u16 => "zenoh/error", | ||
| 1u16 => "zenoh/string", | ||
| 15u16 => "zenoh/serialized", |
There was a problem hiding this comment.
You really want to "reserve" encoding slots?
There was a problem hiding this comment.
I think this makes sense. Later we can even restore primitive types encoding to these slots, with names like zenoh/serialized;u8, zenoh/serialized;i16, etc - when the schema for zenoh encoding is elaborated (this is @kydos 's idea)
There was a problem hiding this comment.
Then you will surely need more than 2..16 range, so why not reserving more? Why do we care having zenoh encodings first? (except for zenoh/bytes which must obviously be 0)
There was a problem hiding this comment.
Mostly because it allows us not to change existing numeration. Yes, it's internal and as we know these codes are not used anywhere, but as it costs nothing to us, better not to break compatibility when we can.
But this is really minor question. If you are strongly against this gap, let's remove it, it's cheaper than discuss it too long.
There was a problem hiding this comment.
@DenisBiryukov91 is also against the gap and this makes zenoh-pico a bit more complex. Then let's renumber them, fixing
milyin
added
the
breaking-change
fixes #1507 also