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 response.text property #420

Merged
merged 1 commit into from
Jun 25, 2015
Merged

fix response.text property #420

merged 1 commit into from
Jun 25, 2015

Conversation

jettify
Copy link
Member

@jettify jettify commented Jun 24, 2015

response.text has unexpected behavior:

response = Rresponse(status=200)
print(response.text)

results in:

File "/home/nick/sources/python/aiohttp/aiohttp/web_reqrep.py", line 743, in text
    return self._body.decode(self.charset or 'utf-8')
nose.proxy.AttributeError: 'NoneType' object has no attribute 'decode '

here tiny fix.

@fafhrd91
Copy link
Member

-1 i do not like to use two different types for same attribute, text should be str only. same for body, it should be bytes only.
i'd prefer to use empty str and bytes as default values

@jettify
Copy link
Member Author

jettify commented Jun 24, 2015

Ok, I've made changes, now body is bytes only and text is str only. For empty payload:b'' and ''

@fafhrd91
Copy link
Member

lgtm from me
@asvetlov ?

@asvetlov
Copy link
Member

We have documented None for body andtext`: http://aiohttp.readthedocs.org/en/stable/web_reference.html#aiohttp.web.Response.text

I'm -0.5 for making it str only, we slightly break backward compatibility.

@fafhrd91 if you insist on converting it to str documentation should be updated.
But I believe None is sigh of body absence while '' is for empty but present http payload.
It may behave differently for gzipped or chunked payload.

@fafhrd91
Copy link
Member

i am +-0 on this. i just do not like to use two different types for one attribute.

@asvetlov
Copy link
Member

I think having None as not specified is good for every basic type: str, int, float etc.

It works like implicit enum Optional<str> for Swift for example -- nil is valid value as well as empty string but these are not equal.

@fafhrd91
Copy link
Member

in swift it is different, you have to explicitly unwrap optional value, and again optional attribute has one type which is enum Optional. but we use two types for text, and bad is that it is implicit, and that is what i do not like.

@asvetlov
Copy link
Member

I still like keeping None as valid value for body and text.
Can you live with that?

@fafhrd91
Copy link
Member

i'm fine with that. but i think this is wrong

@jettify
Copy link
Member Author

jettify commented Jun 25, 2015

reverted to original PR. response.text is None if response.body is None

asvetlov added a commit that referenced this pull request Jun 25, 2015
@asvetlov asvetlov merged commit 4da27ce into aio-libs:master Jun 25, 2015
@asvetlov
Copy link
Member

Thanks, Коля

@lock
Copy link

lock bot commented Oct 30, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Oct 30, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants