Skip to content

Parse multibyte encoding responses #1

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ajgolledge
Copy link
Collaborator

Parse responses encoded in multibyte character sets (e.g. UTF-8) correctly by applying the content length given in the response to the length of the raw byte array and not the encoded string. The length of the string and the length of the byte array are not necessarily equivalent.

Copy link
Member

@pragmatrix pragmatrix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far I think the PR is fine, the only thing that bothers me a bit is that BasicMessage now stores the body string and the body byte array.

…of a UTF-8 string. This byte array is then used for further parsing in the derived EventMessage class to save converting back again to a byte array from a UTF-8 string. Tests have been extended to include a DetectedSpeech event. There are two variants of this event which are used in the tests, one containing a response with English text in the body and another containing German text and umlaut characters. Add Logger initializer to MessageParsingTests class to allow its test cases to be run independently. Add new test case to MessageParsingTests to test for the successful extraction of body payload from the EventMessage.
@ajgolledge ajgolledge force-pushed the parse-multibyte-encoding-responses branch from 8420665 to 74cd2ff Compare June 30, 2021 21:51
@pragmatrix
Copy link
Member

@ajgolledge There is one remaining open conversation here. I think this should be resolved before we merge.

ajgolledge pushed a commit that referenced this pull request Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants