-
-
Notifications
You must be signed in to change notification settings - Fork 40
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 encoding function that returns Data. Fixes #94 #95
base: main
Are you sure you want to change the base?
Conversation
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.
A great start! Just a few nits, nothing major.
var buffer = ByteBufferAllocator().buffer(capacity: 0) | ||
try self.serialize(parts: parts, boundary: boundary, into: &buffer) | ||
return Data(buffer.readableBytesView) |
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.
import NIOFoundationCompat
and use Data(buffer: buffer, byteTransferStrategy: .automatic)
.
And while you're at it, change the String
version to use String(buffer: buffer)
🙂
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.
I used ByteBuffer(data: data)
instead but assume that’s the same.
Co-authored-by: Gwynne Raskind <[email protected]>
Co-authored-by: Gwynne Raskind <[email protected]>
Thank you for your fast feedback! Hopefully I covered everything - please let me know if not or if you have any further comments. |
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.
LGTM - @gwynne anything else to add?
@andrew804 Looks like this is failing in CI |
@0xTim the failing test is the one that tests for encoding and decoding a jpeg image with the original string functions which highlights the original bug ie the original bug is still present, using the string encoding and decoding functions still corrupts the data. Therefore I could either 1) add a warning in the documentation to not use the string functions and instead use the data functions for encoding and decoding images, then just remove the failing test or 2) remove the troublesome string functions entirely and include this PR in the next major release. Unless you have any other suggestion, what would you prefer? |
Added
encodeToData
function toFormDataEncoder
which returns Data instead of String.Added
serializeToData
function toMultipartSerializer
which returns Data instead of String.Added
decode
function toFormDataDecoder
which accepts Data as input.Added test for encoding and decoding a jpeg image with the new Data functions.
Added test for encoding and decoding a jpeg image with the original String functions.
Added image.jpeg in /Utilities
This PR fixes #94