Make BERT encoding aware #1
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
this way we can configure the callbacks to something else at runtime.
This adds an encoding field after the string so that you can apply an encoding to the string sent across the wire.
This commit makes the encoder default to version 1 of the BERT encoding scheme, but allows you to turn on version 2 via a feature flag.
This commit adds two new types, one for unicode strings and one for other encoded strings. Unocide strings have no extra wire protocol overhead, where "other" strings send the encoding name along with the string.
The two new types are extensions, so this commit adds a comment documenting what these extensions are for (namely so that we can support string encodings over the wire).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
I'm just moving mojombo#24 to here.
I don't think we'll be able to upstream that PR since it's technically divergent from the spec (I guess). But I think this patch is important for our app.
/cc @github/systems @brianmario @tmm1 @charliesome @piki @carlosmn