-
Notifications
You must be signed in to change notification settings - Fork 170
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
fallible serialization #1544
base: main
Are you sure you want to change the base?
fallible serialization #1544
Conversation
PR missing one of the required labels: {'internal', 'documentation', 'new feature', 'bug', 'enhancement', 'breaking-change', 'dependencies'} |
PR missing one of the required labels: {'bug', 'dependencies', 'new feature', 'documentation', 'breaking-change', 'enhancement', 'internal'} |
zenoh-ext/src/serialization.rs
Outdated
@@ -33,18 +35,58 @@ impl fmt::Display for ZDeserializeError { | |||
} | |||
impl std::error::Error for ZDeserializeError {} | |||
|
|||
fn default_serialize_n<T: Serialize>(slice: &[T], serializer: &mut ZSerializer) { | |||
#[derive(Debug)] | |||
pub struct ZSerializeError(String); |
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.
Why not using a ZST as it is the case for deserialization?
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.
Why not a Box<dyn Error + Send + Sync>
?
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.
What is a ZST ? About Box - yes, agree, fixing
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.
made it ZST for now
/// See [Zenoh serialization format RFC][1]. | ||
/// | ||
/// [1]: https://github.com/eclipse-zenoh/roadmap/blob/main/rfcs/ALL/Serialization.md#sequences | ||
pub fn try_serialize_iter<T: TrySerialize, I: IntoIterator<Item = T>>( |
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.
Why no try_serialize
method?
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.
added
@milyin At the end we decided to not include this PR in 1.0.0. |
z_try_serialize
function addedTrySerialize
added. The traitSerialize
remains for infallible serializationTrySerialize<Error = Infallible>
Serialize
trait is implemeted automatically for all types which implementsTrySerialize<Error = Infallible>
Serialize
explicitly for types which already implementsTrySerialize<Error = ZSerializeError>
or any other error. This is necessary for e.g.HashMap
CString
added: in demonstration purpose and may be for future use in zenoh-c