-
Notifications
You must be signed in to change notification settings - Fork 20
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
Implement SPICE-0009 External Readers #26
Conversation
96606d9
to
eaed08f
Compare
eaed08f
to
e1f24be
Compare
e1f24be
to
1f7daa3
Compare
5ed4926
to
a1763f4
Compare
…eader with null arguments
b4a209e
to
1ab5471
Compare
case let bytes as [UInt8]: | ||
guard type(of: value) == [UInt8].self else { | ||
fallthrough | ||
} | ||
try self.encode(bytes) |
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.
In Swift, empty arrays succeed for dynamic casting operations to other array types (Array<T>() is Array<U> == true
). In order to correctly fall through for empty non-UInt8
array an additional type comparison using type(of:)
is required. For the fallthrough
to work correctly this case must directly precede the default
case.
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.
TIL!
This should be all set now! Probably best to merge after apple/pkl#882 lands, gets backported, and Pkl 0.27.2 is published and added to pkl-swift CI. |
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.
This mostly looks good! See comments.
Also: can you update the license headers here?
For new files, set year to 2025. For files that have been modified, set year to 2024-2025.
case let bytes as [UInt8]: | ||
guard type(of: value) == [UInt8].self else { | ||
fallthrough | ||
} | ||
try self.encode(bytes) |
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.
TIL!
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!
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.
Looks good!
SPICE-0009
EvaluatorOptions.externalModuleReaders
andEvaluatorOptions.externalResourceReaders
.ExternalReaderClient
to host the child process side of the external reader workflow.