Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(pass-style,marshal): ByteArray, a new binary Passable type #1538
base: markm-to-immutable-3
Are you sure you want to change the base?
feat(pass-style,marshal): ByteArray, a new binary Passable type #1538
Changes from all commits
42075f3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Change to shortlex order. See https://en.wikipedia.org/wiki/Shortlex_order
Shortlex order would enable encodePassable (aka compactOrdered) to encode buffers with a prefix followed by the unescaped bytes. Avoiding escaping would be great.
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.
So this makes ByteArray different than string for ordering?
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.
Yes. For strings, normal lexicographic ordering is overwhelmingly more expected than shortlex. For binaries of different length, I don't think there is any one overwhelming expectation.
FWIW, if I did think we could get away with shortlex for strings too, I would go for it, for the same reason. Needing to insert escapes into bulk data sucks. Attn @gibson042
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.
That would be a breaking change that would have to be opted into at definition of the stores and other components that use the order.
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.
Agreed. That's another reason we cannot get away with 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.
Indeed, because of this breaking change issue, we haven't even moved forward on #2008
Check warning on line 50 in packages/pass-style/src/typeGuards.js
GitHub Actions / lint
Check warning on line 65 in packages/pass-style/src/typeGuards.js
GitHub Actions / lint
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.
If both are byteArrays, then we should delegate the whole thing to
compareRank
. Which, btw, we'll be switching to shortlex order.