-
Notifications
You must be signed in to change notification settings - Fork 295
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
Introduces Bytes
class
#1019
base: main
Are you sure you want to change the base?
Introduces Bytes
class
#1019
Conversation
This introduces a new `Bytes` standard library class, for working with binary data.
@Override | ||
public void visitBytes(VmBytes value) { | ||
throw new VmExceptionBuilder() | ||
.evalError("cannotRenderTypeAddConverter", "DataSize", "Protobuf") |
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/DataSize/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.
Thanks!
["sha256"] { | ||
Bytes("AQIDBA==").sha256 | ||
Bytes("").sha256 | ||
} |
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.
} | |
} | |
["sha256Int"] { | |
Bytes("AQIDBA==").sha256Int | |
Bytes("").sha256Int | |
} |
We've added this to the Bytes
class. We should add coverage for it.
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.
We'll probably want some tests for encodeToStringWithCharset("UTF-8")
and encodeToStringWithCharset("ISO-8859-1")
Bytes(List()) == Bytes("") | ||
Bytes(List(1, 2, 3, 4)) == Bytes("AQIDBA==") | ||
Bytes(List(1, 2)) != Bytes(List(1, 2, 3, 4)) | ||
} |
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.
} | |
} | |
["length"] { | |
Bytes(List()).length == 0 | |
Bytes(List(1, 2, 3, 4)).length == 4 | |
Bytes("AQIDBA==").length == 4 | |
} |
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.
Is it worth also demonstrating that subscripting works?
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.
It actually doesn't work with 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.
The following appears to work for me:
# Input
["subscript"] {
Bytes(List(1, 2, 3, 4))[0] == 1
Bytes(List(1, 2, 3, 4))[3] == 4
}
# Output
["subscript"] {
true
true
}
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 addition to equality, should we also support comparison operations on a sequence of Byte
objects? It is sometimes helpful to lexicographically sort bytes. Java has Byte.compare
for 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.
Ah; that wasn't intentional; that shouldn't work.
If you want to compare individual bytes, you can get them off of its value
. Bytes
is just a wrapper, and not meant to provide things like subscript.
But, if you disagree with this decision, feel free to chime in here: https://github.com/apple/pkl-evolution/pull/14/files#r1984168905
"".isBase64 | ||
"AQIDBA==".isBase64 | ||
!"hello there".isBase64 | ||
} |
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.
} | |
} | |
["toBytesWithCharset"] { | |
"Hello".toBytesWithCharset("UTF-8").length == 5 | |
"Hello".toBytesWithCharset("UTF-16").length > 5 | |
} |
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.
Love the enthusiasm, but FYI: this is just a draft PR! It's not quite ready for review yet.
} catch (UnsupportedEncodingException e) { | ||
throw PklBugException.unreachableCode(); | ||
} |
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.
Should we have a more specific exception than just unreachableCode
here? We're handed an UnsupportedEncodingException
.
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 is an impossible condition; all JVM distributions support UTF-8, UTF-16 and ISO-8859-1
This introduces a new
Bytes
standard library class, for working with binary data.Follows SPICE-0013