-
Notifications
You must be signed in to change notification settings - Fork 2
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/decoding of bit array to string #1
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.
Thanks for the contribution. It is cool. I am not very fond of the asymmetry introduced by instance method vs. static method, though.
BitArray.decodeWithCharacterSet(myarray.encodeWithCharacterSet(charset), charset)
I would rather go for utility functions:
decode(encode(myarray, charset), charset)
This would be more consistent with native encoding utilities such as btoa
/atob
or encodeURI
/decodeURI
; and it would work well with tree-shaking.
This can be achieved by named exports. So I would suggest:
// we could even make charSet optional and default to base64MIMEChars
export function encode(arr: BitArray, charSet = base64MIMEChars) {
// your code here
}
export function decode(encodedString: string, charSet = base64MIMEChars) {
// your code here
}
Other minor comments inline. That being said, I generally like your proposal 👍
@swiing Mulled it over a bit...really this logic belongs just on ArrayBuffer after all I think, most of which exists in other packages. But that leaves the issue of reconstructing a BitArray as a view on an ArrayBuffer. So maybe this PR isn't needed at all :P |
Indeed, it has little to do with ArrayBuffers 😀 |
Fixes- swiing/Bit-TypedArray#5Added general functions for encoding/decoding with appropriate documentation and tests; added 2 helper pairs for commonly used variants of base64 encoding.