Repository: huge-success/sanic
Pull request #1739 (2019-12-24 04:55:09)

Deprecate body_bytes to merge into body
As suggested in https://github.com/huge-success/sanic/pull/1736#issuecomment-568266049, this PR deprecates `body_bytes` and merges it into `body`.

Please note that I have intentionally removed the `body_bytes` param completely to elicit discussion about how to properly deprecate it (cc @ahopkins who has some thoughts to share).

codecov[bot]: # [Codecov](https://codecov.io/gh/huge-success/sanic/pull/1739?src=pr&el=h1) Report
> Merging [#1739](https://codecov.io/gh/huge-success/sanic/pull/1739?src=pr&el=desc) into [master](https://codecov.io/gh/huge-success/sanic/commit/243f240e5f3f929c9d373e06de390f3177eb7466?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/huge-success/sanic/pull/1739/graphs/tree.svg?width=650&token=NqLbJHUqoR&height=150&src=pr)](https://codecov.io/gh/huge-success/sanic/pull/1739?src=pr&el=tree)

```diff
@@           Coverage Diff           @@
##           master    #1739   +/-   ##
=======================================
  Coverage   92.18%   92.18%           
=======================================
  Files          22       22           
  Lines        2238     2238           
  Branches      419      419           
=======================================
  Hits         2063     2063           
  Misses        136      136           
  Partials       39       39
```


| [Impacted Files](https://codecov.io/gh/huge-success/sanic/pull/1739?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [sanic/response.py](https://codecov.io/gh/huge-success/sanic/pull/1739/diff?src=pr&el=tree#diff-c2FuaWMvcmVzcG9uc2UucHk=) | `100% <100%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/huge-success/sanic/pull/1739?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/huge-success/sanic/pull/1739?src=pr&el=footer). Last update [243f240...bc5925b](https://codecov.io/gh/huge-success/sanic/pull/1739?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

Tronic: Check out #1674 as there is some overlap, in particular with how bytes vs. other types are detected.
LiraNuna: Sure, I can upgrade body to bytes but that will keep the weirdness that is the two parameters merging into the same thing. 

Here are the questions I have in a more blunt form:
* How will the developer know to stop using `body_bytes` and to start using `body`?
* When will `body_bytes` be removed?
* What happens if both `body` and `body_bytes` are passed in?
* And what's up with `black` adding comma to line 208?
codecov[bot]: # [Codecov](https://codecov.io/gh/huge-success/sanic/pull/1739?src=pr&el=h1) Report
> Merging [#1739](https://codecov.io/gh/huge-success/sanic/pull/1739?src=pr&el=desc) into [master](https://codecov.io/gh/huge-success/sanic/commit/bc5925b4c9ac4cf21c18d2ffc441dae055ae27ba?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/huge-success/sanic/pull/1739/graphs/tree.svg?width=650&token=NqLbJHUqoR&height=150&src=pr)](https://codecov.io/gh/huge-success/sanic/pull/1739?src=pr&el=tree)

```diff
@@           Coverage Diff           @@
##           master    #1739   +/-   ##
=======================================
  Coverage   92.18%   92.18%           
=======================================
  Files          22       22           
  Lines        2238     2238           
  Branches      419      419           
=======================================
  Hits         2063     2063           
  Misses        136      136           
  Partials       39       39
```



------

[Continue to review full report at Codecov](https://codecov.io/gh/huge-success/sanic/pull/1739?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/huge-success/sanic/pull/1739?src=pr&el=footer). Last update [bc5925b...6fe26fc](https://codecov.io/gh/huge-success/sanic/pull/1739?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

LiraNuna: @Tronic, my main questions are "is it okay to deprecate this parameter?" and "how will library users know to avoid code breakage?"

I'm assuming a version bump and a note in the CHANGELOG, but is it this PR's responsibility to do that? 

I think this PR is ready to review assuming the param deprecation is wanted.
ahopkins: Sorry for not jumping in with thoughts sooner. I was sort of waiting for [the new organizational structure](https://sanicframework.org/2020-release-managers-and-a-new-oraganizational-structure) to kick in and let there be a bit more of a formalized discussion about how to handle deprecations.

I would like to put this one on hold until that conversation takes place. I expect the Steering Council to be in place within the next week or two.
Tronic: @ahopkins Any updates on that? I'd like to see this merged for 20.3.

LiraNuna: What is stopping this PR from being merged? I'd love to get it over the finish line
Tronic: Alternatively, modify this PR so that it still takes body_bytes arguments but gives a warning whenever those are used.
LiraNuna: @Tronic how does this look? I chose to accept the old param so when the true deprecation comes, there are more options

Patch:
diff --git a/sanic/response.py b/sanic/response.py
index 37d58304..6f02a35c 100644
--- a/sanic/response.py
+++ b/sanic/response.py
@@ -149,6 +149,12 @@ class HTTPResponse(BaseHTTPResponse):
         self.headers = Header(headers or {})
         self._cookies = None
 
+        if body_bytes:
+            warnings.warn(
+                "Parameter `body_bytes` is deprecated, use `body` instead",
+                DeprecationWarning,
+            )
+
     def output(self, version="1.1", keep_alive=False, keep_alive_timeout=None):
         body = b""
         if has_message_body(self.status):
@@ -173,7 +179,7 @@ def empty(status=204, headers=None):
     :param status Response code.
     :param headers Custom Headers.
     """
-    return HTTPResponse(body_bytes=b"", status=status, headers=headers)
+    return HTTPResponse(body=b"", status=status, headers=headers)
 
 
 def json(
@@ -243,10 +249,7 @@ def raw(
     :param content_type: the content type (string) of the response.
     """
     return HTTPResponse(
-        body_bytes=body,
-        status=status,
-        headers=headers,
-        content_type=content_type,
+        body=body, status=status, headers=headers, content_type=content_type,
     )
 
 
@@ -306,10 +309,10 @@ async def file(
 
     mime_type = mime_type or guess_type(filename)[0] or "text/plain"
     return HTTPResponse(
+        body=out_stream,
         status=status,
         headers=headers,
         content_type=mime_type,
-        body_bytes=out_stream,
     )
 
 


Test patch:
diff --git a/tests/test_response.py b/tests/test_response.py
index a3cb93c0..d3ed294b 100644
--- a/tests/test_response.py
+++ b/tests/test_response.py
@@ -1,6 +1,7 @@
 import asyncio
 import inspect
 import os
+import warnings
 
 from collections import namedtuple
 from mimetypes import guess_type
@@ -607,3 +608,17 @@ def test_empty_response(app):
     request, response = app.test_client.get("/test")
     assert response.content_type is None
     assert response.body == b""
+
+
+def test_response_body_bytes_deprecated(app):
+    with warnings.catch_warnings(record=True) as w:
+        warnings.simplefilter("always")
+
+        HTTPResponse(body_bytes=b'bytes')
+
+        assert len(w) == 1
+        assert issubclass(w[0].category, DeprecationWarning)
+        assert (
+            "Parameter `body_bytes` is deprecated, use `body` instead"
+            in str(w[0].message)
+        )