Skip to content

Better string handling for dogwrap and python3 support#379

Merged
zippolyte merged 8 commits intomasterfrom
hippo/encoding
Jun 3, 2019
Merged

Better string handling for dogwrap and python3 support#379
zippolyte merged 8 commits intomasterfrom
hippo/encoding

Conversation

@zippolyte
Copy link
Copy Markdown
Contributor

@zippolyte zippolyte commented May 28, 2019

  • Arguments are bytes, so the join must be made on a binary string, otherwise python will try to convert and can encounter errors.
  • Stop decoding lines before writing to stdout, since write expects bytes.
  • Move decoding the output to the end of the execution, to make sure we won't raise exceptions while command is still running, which can cause unexpected behavior, like in issue Dogwrap doesn't support unicode characters #371 .

Resolves #371, closes #223

Copy link
Copy Markdown
Contributor

@gzussa gzussa left a comment

Choose a reason for hiding this comment

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

Much better!! and clean description. Thanks for this fix! Should we add tests to make sure it solves the issue?

@synth
Copy link
Copy Markdown

synth commented May 31, 2019

Sorry for the delay in testing, but got to it today. It works! In our testing, dogwrap stays alive and no more database breakdown on unicode output! +1 on adding tests ;-) We'd love for this to get merged to master and available via pip install :)

@zippolyte zippolyte merged commit b829d13 into master Jun 3, 2019
@zippolyte zippolyte deleted the hippo/encoding branch June 3, 2019 09:24
@zippolyte
Copy link
Copy Markdown
Contributor Author

Thanks for testing this @synth, we'll likely do a release on PyPI this week

@zippolyte zippolyte changed the title Better string handling for dogwrap Better string handling for dogwrap and python3 support Jun 4, 2019
dabcoder pushed a commit to dabcoder/datadogpy that referenced this pull request Oct 25, 2019
dabcoder pushed a commit to dabcoder/datadogpy that referenced this pull request Nov 28, 2019
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.

Dogwrap doesn't support unicode characters API client can't handle 'ascii' codec.

3 participants