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

[rest] change text from a property to a method #20290

Merged
merged 9 commits into from
Aug 20, 2021
5 changes: 3 additions & 2 deletions sdk/core/azure-core/azure/core/rest/_requests_basic.py
Original file line number Diff line number Diff line change
Expand Up @@ -85,10 +85,11 @@ def encoding(self, value):
self._encoding = value
self._internal_response.encoding = value

@property
def text(self):
def text(self, encoding=None):
# this will trigger errors if response is not read in
self.content # pylint: disable=pointless-statement
if encoding:
self._internal_response.encoding = encoding
xiangyan99 marked this conversation as resolved.
Show resolved Hide resolved
return self._internal_response.text

def _stream_download_helper(decompress, response):
Expand Down
21 changes: 13 additions & 8 deletions sdk/core/azure-core/azure/core/rest/_rest.py
Original file line number Diff line number Diff line change
Expand Up @@ -235,17 +235,23 @@ def encoding(self, value):
# type: (str) -> None
"""Sets the response encoding"""
self._encoding = value
self._text = None # clear text cache

@property
def text(self):
# type: (...) -> str
"""Returns the response body as a string"""
if self._text is None:
def text(self, encoding=None):
# type: (Optional[str]) -> str
"""Returns the response body as a string

:param optional[str] encoding: The encoding you want to decode the text with. Can
also be set independently through our encoding property
:return: The response's content decoded as a string.
"""
if self._text is None or encoding:
xiangyan99 marked this conversation as resolved.
Show resolved Hide resolved
content = self.content
if not content:
self._text = ""
else:
self._text = decode_to_text(self.encoding, self.content)
encoding_to_pass = encoding or self.encoding
self._text = decode_to_text(encoding_to_pass, self.content)
return self._text

def json(self):
Expand All @@ -259,7 +265,7 @@ def json(self):
# this will trigger errors if response is not read in
self.content # pylint: disable=pointless-statement
if not self._json:
self._json = loads(self.text)
self._json = loads(self.text())
return self._json

def raise_for_status(self):
Expand Down Expand Up @@ -314,7 +320,6 @@ class HttpResponse(_HttpResponseBase): # pylint: disable=too-many-instance-attr
:ivar str text: The response body as a string.
:ivar request: The request that resulted in this response.
:vartype request: ~azure.core.rest.HttpRequest
:ivar internal_response: The object returned from the HTTP library.
:ivar str content_type: The content type of the response
:ivar bool is_closed: Whether the network connection has been closed yet
:ivar bool is_stream_consumed: When getting a stream response, checks
Expand Down
21 changes: 12 additions & 9 deletions sdk/core/azure-core/azure/core/rest/_rest_py3.py
Original file line number Diff line number Diff line change
Expand Up @@ -262,16 +262,22 @@ def encoding(self) -> Optional[str]:
def encoding(self, value: str) -> None:
"""Sets the response encoding"""
self._encoding = value
self._text = None # clear text cache

@property
def text(self) -> str:
"""Returns the response body as a string"""
if self._text is None:
def text(self, encoding: Optional[str] = None) -> str:
"""Returns the response body as a string

:param optional[str] encoding: The encoding you want to decode the text with. Can
also be set independently through our encoding property
:return: The response's content decoded as a string.
"""
if self._text is None or encoding:
content = self.content
if not content:
self._text = ""
else:
self._text = decode_to_text(self.encoding, self.content)
encoding_to_pass = encoding or self.encoding
self._text = decode_to_text(encoding_to_pass, self.content)
Copy link
Member

Choose a reason for hiding this comment

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

Why use self.content when you fished out the value on line 275. Alternatively, why are you fishing out content on line 275? Do we want to save the instructions needed to access the attribute from the instance?

Copy link
Member

Choose a reason for hiding this comment

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

Is it because accessing the content property has side-effects?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Acccessign the content property will raise the ResponseNotRead errors if users mishandle. You ahve a good point, I don't have to store it in another variable, so I'll just stick to using self.content

Copy link
Member

Choose a reason for hiding this comment

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

I agree with pylint that the side effects of accessing self.content are too subtle. You are going to access self.content anyways, why do you need it also call it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry i'm not quite understanding your comment. Does the current code there look better to you?

Copy link
Member

Choose a reason for hiding this comment

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

Why not just say:

if not self.content:  # <- this will raise if data is not read. Line 275 is not needed. 
    self._text = ""
else:
    ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah i think i was confused bc we're looking at diff code versions. What I currently have is

if self._text is None or encoding:
    encoding_to_pass = encoding or self.encoding
    self._text = decode_to_text(encoding_to_pass, self.content)
return self._text

which i think solves the issue you brought up (i moved the empty content detection to decode_to_text

return self._text

def json(self) -> Any:
Expand All @@ -284,7 +290,7 @@ def json(self) -> Any:
# this will trigger errors if response is not read in
self.content # pylint: disable=pointless-statement
if not self._json:
self._json = loads(self.text)
self._json = loads(self.text())
return self._json

def raise_for_status(self) -> None:
Expand Down Expand Up @@ -328,7 +334,6 @@ class HttpResponse(_HttpResponseBase):
:ivar str text: The response body as a string.
:ivar request: The request that resulted in this response.
:vartype request: ~azure.core.rest.HttpRequest
:ivar internal_response: The object returned from the HTTP library.
:ivar str content_type: The content type of the response
:ivar bool is_closed: Whether the network connection has been closed yet
:ivar bool is_stream_consumed: When getting a stream response, checks
Expand Down Expand Up @@ -421,7 +426,6 @@ class AsyncHttpResponse(_HttpResponseBase):

:keyword request: The request that resulted in this response.
:paramtype request: ~azure.core.rest.HttpRequest
:keyword internal_response: The object returned from the HTTP library.
:ivar int status_code: The status code of this response
:ivar mapping headers: The response headers
:ivar str reason: The reason phrase for this response
Expand All @@ -432,7 +436,6 @@ class AsyncHttpResponse(_HttpResponseBase):
:ivar str text: The response body as a string.
:ivar request: The request that resulted in this response.
:vartype request: ~azure.core.rest.HttpRequest
:ivar internal_response: The object returned from the HTTP library.
:ivar str content_type: The content type of the response
:ivar bool is_closed: Whether the network connection has been closed yet
:ivar bool is_stream_consumed: When getting a stream response, checks
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
async def test_normal_call(client):
async def _raise_and_get_text(response):
response.raise_for_status()
assert response.text == "Hello, world!"
assert response.text() == "Hello, world!"
assert response.is_closed
request = HttpRequest("GET", url="/basic/string")
response = await client.send_request(request)
Expand All @@ -33,9 +33,9 @@ async def _raise_and_get_text(response):
response.raise_for_status()
assert not response.is_closed
with pytest.raises(ResponseNotReadError):
response.text
response.text()
await response.read()
assert response.text == "Hello, world!"
assert response.text() == "Hello, world!"
assert response.is_closed
request = HttpRequest("GET", url="/streams/basic")
response = await client.send_request(request, stream=True)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,10 @@
@pytest.fixture
def send_request(client):
async def _send_request(request):
response = await client.send_request(request, stream=False)
response.raise_for_status()
return response
async with client:
response = await client.send_request(request, stream=False)
response.raise_for_status()
return response
return _send_request

@pytest.mark.asyncio
Expand All @@ -27,7 +28,7 @@ async def test_response(send_request, port):
assert response.status_code == 200
assert response.reason == "OK"
assert response.content == b"Hello, world!"
assert response.text == "Hello, world!"
assert response.text() == "Hello, world!"
assert response.request.method == "GET"
assert response.request.url == "http://localhost:{}/basic/string".format(port)

Expand All @@ -40,7 +41,7 @@ async def test_response_content(send_request):
assert response.reason == "OK"
content = await response.read()
assert content == b"Hello, world!"
assert response.text == "Hello, world!"
assert response.text() == "Hello, world!"

@pytest.mark.asyncio
async def test_response_text(send_request):
Expand All @@ -51,7 +52,7 @@ async def test_response_text(send_request):
assert response.reason == "OK"
content = await response.read()
assert content == b"Hello, world!"
assert response.text == "Hello, world!"
assert response.text() == "Hello, world!"
assert response.headers["Content-Length"] == '13'
assert response.headers['Content-Type'] == "text/plain; charset=utf-8"

Expand All @@ -64,7 +65,7 @@ async def test_response_html(send_request):
assert response.reason == "OK"
content = await response.read()
assert content == b"<html><body>Hello, world!</html></body>"
assert response.text == "<html><body>Hello, world!</html></body>"
assert response.text() == "<html><body>Hello, world!</html></body>"

@pytest.mark.asyncio
async def test_raise_for_status(client):
Expand Down Expand Up @@ -106,7 +107,7 @@ async def test_response_content_type_encoding(send_request):
await response.read()
assert response.content_type == "text/plain; charset=latin-1"
assert response.content == b'Latin 1: \xff'
assert response.text == "Latin 1: ÿ"
assert response.text() == "Latin 1: ÿ"
assert response.encoding == "latin-1"


Expand All @@ -119,7 +120,7 @@ async def test_response_autodetect_encoding(send_request):
request=HttpRequest("GET", "/encoding/latin-1")
)
await response.read()
assert response.text == u'Latin 1: ÿ'
assert response.text() == u'Latin 1: ÿ'
assert response.encoding == "latin-1"


Expand All @@ -133,7 +134,7 @@ async def test_response_fallback_to_autodetect(send_request):
)
await response.read()
assert response.headers["Content-Type"] == "text/plain; charset=invalid-codec-name"
assert response.text == "おはようございます。"
assert response.text() == "おはようございます。"
assert response.encoding is None


Expand All @@ -152,7 +153,7 @@ async def test_response_no_charset_with_ascii_content(send_request):
assert response.encoding is None
content = await response.read()
assert content == b"Hello, world!"
assert response.text == "Hello, world!"
assert response.text() == "Hello, world!"


@pytest.mark.asyncio
Expand All @@ -165,7 +166,7 @@ async def test_response_no_charset_with_iso_8859_1_content(send_request):
request=HttpRequest("GET", "/encoding/iso-8859-1"),
)
await response.read()
assert response.text == "Accented: �sterreich" # aiohttp is having diff behavior than requests
assert response.text() == "Accented: �sterreich" # aiohttp is having diff behavior than requests
assert response.encoding is None

# NOTE: aiohttp isn't liking this
Expand All @@ -177,7 +178,7 @@ async def test_response_no_charset_with_iso_8859_1_content(send_request):
# assert response.headers["Content-Type"] == "text/plain; charset=utf-8"
# response.encoding = "latin-1"
# await response.read()
# assert response.text == "Latin 1: ÿ"
# assert response.text() == "Latin 1: ÿ"
# assert response.encoding == "latin-1"

@pytest.mark.asyncio
Expand All @@ -204,23 +205,23 @@ async def test_emoji(send_request):
request=HttpRequest("GET", "/encoding/emoji"),
)
await response.read()
assert response.text == "👩"
assert response.text() == "👩"

@pytest.mark.asyncio
async def test_emoji_family_with_skin_tone_modifier(send_request):
response = await send_request(
request=HttpRequest("GET", "/encoding/emoji-family-skin-tone-modifier"),
)
await response.read()
assert response.text == "👩🏻‍👩🏽‍👧🏾‍👦🏿 SSN: 859-98-0987"
assert response.text() == "👩🏻‍👩🏽‍👧🏾‍👦🏿 SSN: 859-98-0987"

@pytest.mark.asyncio
async def test_korean_nfc(send_request):
response = await send_request(
request=HttpRequest("GET", "/encoding/korean"),
)
await response.read()
assert response.text == "아가"
assert response.text() == "아가"

@pytest.mark.asyncio
async def test_urlencoded_content(send_request):
Expand Down Expand Up @@ -249,9 +250,25 @@ async def test_send_request_return_pipeline_response(client):
assert hasattr(response, "http_request")
assert hasattr(response, "http_response")
assert hasattr(response, "context")
assert response.http_response.text == "Hello, world!"
assert response.http_response.text() == "Hello, world!"
assert hasattr(response.http_request, "content")

@pytest.mark.asyncio
async def test_text_and_encoding(send_request):
response = await send_request(
request=HttpRequest("GET", "/encoding/emoji"),
)
assert response.content == u"👩".encode("utf-8")
assert response.text() == u"👩"

# try setting encoding as a property
response.encoding = "utf-16"
assert response.text() == u"鿰ꦑ" == response.content.decode(response.encoding)

# assert latin-1 changes text decoding without changing encoding property
assert response.text("latin-1") == 'ð\x9f\x91©' == response.content.decode("latin-1")
assert response.encoding == "utf-16"

# @pytest.mark.asyncio
# async def test_multipart_encode_non_seekable_filelike(send_request):
# """
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -170,11 +170,11 @@ async def test_iter_read_back_and_forth(client):
async for line in response.iter_lines():
assert line
with pytest.raises(ResponseNotReadError):
response.text
response.text()
with pytest.raises(StreamConsumedError):
await response.read()
with pytest.raises(ResponseNotReadError):
response.text
response.text()

@pytest.mark.asyncio
async def test_stream_with_return_pipeline_response(client):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
def test_normal_call(client, port):
def _raise_and_get_text(response):
response.raise_for_status()
assert response.text == "Hello, world!"
assert response.text() == "Hello, world!"
assert response.is_closed
request = HttpRequest("GET", url="/basic/string")
response = client.send_request(request)
Expand All @@ -30,9 +30,9 @@ def _raise_and_get_text(response):
response.raise_for_status()
assert not response.is_closed
with pytest.raises(ResponseNotReadError):
response.text
response.text()
response.read()
assert response.text == "Hello, world!"
assert response.text() == "Hello, world!"
assert response.is_closed
request = HttpRequest("GET", url="/streams/basic")
response = client.send_request(request, stream=True)
Expand Down
Loading