Skip to content

Some bytes/str issues in py3 w/ zlib and json#2034

Closed
wyndhblb wants to merge 25 commits into
apache:masterfrom
wyndhblb:master
Closed

Some bytes/str issues in py3 w/ zlib and json#2034
wyndhblb wants to merge 25 commits into
apache:masterfrom
wyndhblb:master

Conversation

@wyndhblb
Copy link
Copy Markdown
Contributor

zlib wants bytes, json wants strings ...

@bkyryliuk
Copy link
Copy Markdown
Member

@wyndhblb - could u please add a test to demonstrate the issue?
It would be great to prevent the regression.

@wyndhblb
Copy link
Copy Markdown
Contributor Author

sure let me refactor things a bit first

@xrmx
Copy link
Copy Markdown
Contributor

xrmx commented Jan 25, 2017

I think it would be better that instead of being sloppy on how we call functions and adding wrappers that handles all the the types we just have more care and pass the right types around.

@wyndhblb
Copy link
Copy Markdown
Contributor Author

sure (python 2+3 makes that hard w/ the type differences) ... six could help, but the default six.b/u does latin-1 and that's not a good encoding default for most modern DBs.

Comment thread tests/utils_tests.py Outdated
json_str = """{"test": 1}"""
blob = zlib_compress(json_str)
got_str = zlib_uncompress_to_string(blob)
self.assertEquals(json_str, got_str) No newline at end of file
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

NIT: missing return char at EOF

@mistercrunch
Copy link
Copy Markdown
Member

This LGTM though there's a conflict, please rebase.

[optional] These pure function are nice to test as doctests so that you have an example that is also a test in the docstring. The way things are hooked up it's also accounted for in code coverage analysis.

# Conflicts:
#	superset/utils.py
#	superset/views.py
@mistercrunch
Copy link
Copy Markdown
Member

can we keep the formatting things outside this PR, codeclimate doesn't seem to agree with your auto-formatter.

@rumbin
Copy link
Copy Markdown
Contributor

rumbin commented Apr 5, 2017

What can be done to get this merged? This bug is effectively keeping me from switching to Python 3 (see #2079), which I would love to do, as Python 2 has its encoding issues of its own that I'm struggling with.

@mistercrunch
Copy link
Copy Markdown
Member

This PR needs to:

  • rebase
  • remove all of the unnecessary formatting/indent changes
  • pass the build

@rumbin
Copy link
Copy Markdown
Contributor

rumbin commented Apr 5, 2017

See alternative PR #2558, as this one doesn't seem to move on.

@mistercrunch
Copy link
Copy Markdown
Member

superseeded

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.

5 participants