Skip to content
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

Fix all BytesWarning caught during tests #5561

Merged
merged 1 commit into from Nov 6, 2017
Merged

Fix all BytesWarning caught during tests #5561

merged 1 commit into from Nov 6, 2017

Conversation

jdufresne
Copy link
Contributor

@jdufresne jdufresne commented Nov 5, 2017

Running the tests with bytes warning enabled shows some bytes/str mixups. Fix them all.

Some examples of mixing usage:

str(b'foo') -- calling str() on bytes
b'foo' == 'foo' -- compare str with bytes
'foo' + b'bar' -- concatenating str and bytes

Running the tests with bytes warning enabled shows some bytes/str
mixups. Fix them all.

Some examples of mixing usage:

str(b'foo') -- calling str() on bytes
b'foo' == 'foo' -- compare str with bytes
'foo' + b'bar' -- concatenating str and bytes
@@ -676,7 +676,7 @@ def get(self, request):
request = factory.get('/')
response = view(request)
response.render()
self.assertInHTML('<tr><th>Foo</th><td>a string</td></tr>', str(response.content))
self.assertContains(response, '<tr><th>Foo</th><td>a string</td></tr>', html=True)
Copy link
Contributor

@auvipy auvipy Nov 6, 2017

Choose a reason for hiding this comment

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

how about using pytest assert style?

Copy link
Contributor Author

@jdufresne jdufresne Nov 6, 2017

Choose a reason for hiding this comment

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

I'm happy to take a look at this suggested change, but I'm not very familiar with all the features of pytest. Do you have a link to documentation on this feature or a short example?

@xordoquy
Copy link
Collaborator

xordoquy commented Nov 6, 2017

I'm a bit surprised by this change. I don't really get how it works with a missing decode. Python3 enforces a strict bytes / string conversion. How does it work without ?

@carltongibson carltongibson added this to the 3.7.2 milestone Nov 6, 2017
Copy link
Collaborator

@carltongibson carltongibson left a comment

OK. This looks good. Thank you!

@carltongibson carltongibson merged commit f77e794 into encode:master Nov 6, 2017
1 check passed
@@ -83,5 +83,5 @@ def precedence(self):
def __str__(self):
ret = "%s/%s" % (self.main_type, self.sub_type)
for key, val in self.params.items():
ret += "; %s=%s" % (key, val)
ret += "; %s=%s" % (key, val.decode('ascii'))
Copy link
Collaborator

@xordoquy xordoquy Nov 6, 2017

Choose a reason for hiding this comment

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

I still don't get why this is required. I'm afraid we create a regression here.

Copy link
Contributor Author

@jdufresne jdufresne Nov 6, 2017

Choose a reason for hiding this comment

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

The %s placeholder implies a call to str() before substitution. In Python3, this looks like:

>>> >>> print('%s' % b'foo')
b'foo'

So, the end result of the formatted string is something like "test/*; foo=b'bar'". Notice the extra characters b''. My understanding is this repr bytes value was not intended as part of the return value. If true, this wasn't previously correct. Enabling BytesWarning caught this bug. This was caught by fixing the test test_mediatype_string_representation().

Copy link
Collaborator

@xordoquy xordoquy Nov 6, 2017

Choose a reason for hiding this comment

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

@jdufresne thanks for the feedback. Indeed, you're correct. Thanks for the nice work !

@@ -567,6 +567,8 @@ def get_raw_data_form(self, data, view, method, request):
if isinstance(field, serializers.HiddenField):
data.pop(name, None)
content = renderer.render(data, accepted, context)
# Renders returns bytes, but CharField expects a str.
content = content.decode('utf-8')
Copy link
Collaborator

@xordoquy xordoquy Nov 6, 2017

Choose a reason for hiding this comment

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

I still don't get why this is required. I'm afraid we create a regression here.

Copy link
Collaborator

@carltongibson carltongibson Nov 6, 2017

Choose a reason for hiding this comment

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

If you run with the -b flag we get a BytesWarning from the (Django) widget rendering the value here. The conversion to str resolves this.

There’s no issue on Python 2.7. You decode bytes into Unicode string. e.g.:

>>> "abc".decode('utf-8')
u'abc'

Copy link
Contributor Author

@jdufresne jdufresne Nov 6, 2017

Choose a reason for hiding this comment

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

Further, the Django widget compares the value as a str. In Python3 comparing bytes with str will always result in False and is normally not what was intended. I think that is true here. The CharField was implemented to handle str inputs, not bytes.

>>> b'' == ''
False

@jdufresne jdufresne deleted the bytes-warning branch Nov 9, 2017
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.

None yet

4 participants