-
Notifications
You must be signed in to change notification settings - Fork 466
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 partial support to the binary decoding initializer. #290
Conversation
Added second commit with docs. |
4939b2d
to
af68441
Compare
@@ -268,7 +268,20 @@ public extension Message { | |||
return visitor.serializedSize | |||
} | |||
|
|||
init(serializedData data: Data, extensions: ExtensionSet? = nil) throws { | |||
/// Initializes the message by decoding the Protocol Buffer binary serialization | |||
/// format into for this message. |
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.
remove "into"
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.
Done.
/// format into for this message. | ||
/// | ||
/// - Parameters: | ||
/// - serializedData: The binary serialation data to decode. |
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.
s/serialation/serialization/
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.
Done
/// | ||
/// - Parameters: | ||
/// - serializedData: The binary serialation data to decode. | ||
/// - extensions: An `ExtensionSet` to log up and decode any extensions in this |
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.
s/log/look/?
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.
Yup, done.
/// - Parameters: | ||
/// - serializedData: The binary serialation data to decode. | ||
/// - extensions: An `ExtensionSet` to log up and decode any extensions in this | ||
/// message or message nested within this message's fields. |
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.
"message or messages nested" (add "s")
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.
done
/// - serializedData: The binary serialation data to decode. | ||
/// - extensions: An `ExtensionSet` to log up and decode any extensions in this | ||
/// message or message nested within this message's fields. | ||
/// - partial: The binary serialization format requires all `required` fields |
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.
"By default, the binary serialization format..."
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.
Done.
/// message or message nested within this message's fields. | ||
/// - partial: The binary serialization format requires all `required` fields | ||
/// be present; when `partial` is `false`, | ||
/// `BinaryDecodingError.missingRequiredFields` is throw if any were missing. |
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.
s/throw/thrown/
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.
Done.
/// be present; when `partial` is `false`, | ||
/// `BinaryDecodingError.missingRequiredFields` is throw if any were missing. | ||
/// When `partial` is `true`, then partial messages are allowed, and | ||
/// `Message.isRequired` is not checked. |
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.
s/isRequired/isInitialized/
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.
Doh.
/// `BinaryDecodingError.missingRequiredFields` is throw if any were missing. | ||
/// When `partial` is `true`, then partial messages are allowed, and | ||
/// `Message.isRequired` is not checked. | ||
/// - Throws: An instance of `BinaryDecodingError` on failure . |
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.
remove space before period
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.
Done.
@@ -23,4 +23,8 @@ public enum BinaryDecodingError: Error { | |||
case malformedProtobuf | |||
/// The data being parsed does not match the type specified in the proto file | |||
case schemaMismatch | |||
/// The message or sub messages definitions have required fields, and the |
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.
s/sub/nested/
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.
Done.
Also add tests to confirm partial and full decoding work as expected.
af68441
to
5571ce8
Compare
Also add tests to confirm partial and full decoding work as
expected.