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

Fixed #30294 -- Allowed HttpResponse to accept memoryview objects. #11133

Merged
merged 1 commit into from Mar 29, 2019

Conversation

laymonage
Copy link
Contributor

@laymonage laymonage commented Mar 27, 2019

Ticket #30294.

@carltongibson
Copy link
Member

@carltongibson carltongibson commented Mar 27, 2019

Hi @laymonage. We need a test case showing the error. (It should fail before the patch and pass afterwards.) Are you able to add this?

@laymonage
Copy link
Contributor Author

@laymonage laymonage commented Mar 27, 2019

Done! @carltongibson

@carltongibson
Copy link
Member

@carltongibson carltongibson commented Mar 27, 2019

Hi @laymonage. Super. Thanks.

Can I ask you to move the test to tests/httpwrappers, underneath this one say?

Other than that, this looks super.

First time contributor

Welcome aboard! 🎁

@carltongibson
Copy link
Member

@carltongibson carltongibson commented Mar 27, 2019

(When moving it, can you compress the test slightly to match the style of the other tests there? Thanks!)

@laymonage laymonage force-pushed the ticket_30294 branch 2 times, most recently from b802121 to 8a0d8ca Compare Mar 27, 2019
@laymonage
Copy link
Contributor Author

@laymonage laymonage commented Mar 27, 2019

You're welcome! Sorry for the force-push, I decided to rename the test case as well and I didn't think it's worth a commit.

@laymonage
Copy link
Contributor Author

@laymonage laymonage commented Mar 27, 2019

Looks like one of the builds need to be restarted...

@carltongibson
Copy link
Member

@carltongibson carltongibson commented Mar 27, 2019

Hi @laymonage. Super thank you.

Sorry for the force-push...

No worries, perfect.

If you want bonus points, can you squash to a single commit and match the required commit message format? Something like:

Fixed #30294 -- Allowed HttpResponse to accept memoryview objects.

@laymonage laymonage changed the title Fixed #30294 -- Handle memoryview objects in HttpResponse Fixed #30294 -- Allowed HttpResponse to accept memoryview objects. Mar 27, 2019
@laymonage
Copy link
Contributor Author

@laymonage laymonage commented Mar 27, 2019

Done! 😄

@carltongibson
Copy link
Member

@carltongibson carltongibson commented Mar 27, 2019

Thanks @laymonage. (Just in conference with @felixxm as to docs/release notes for this: current docs for HttpResponse don't quite seem right as are... — will follow-up with you shortly.)

@felixxm
Copy link
Member

@felixxm felixxm commented Mar 27, 2019

I added PR with documentation fix #11134.

@carltongibson
Copy link
Member

@carltongibson carltongibson commented Mar 28, 2019

Hi @laymonage. So, this will be a new feature for v3.0, adding memoryview compatibility to HttpResonse. As such, we'll need an entry in docs/releases/3.0.txt. Do you fancy adding that? (Let me know if you want/need a hand.)

@laymonage
Copy link
Contributor Author

@laymonage laymonage commented Mar 28, 2019

Hi @carltongibson. Should I file it under Minor features and add a new django.http.response entry?

@carltongibson
Copy link
Member

@carltongibson carltongibson commented Mar 28, 2019

@laymonage: How about in "Requests and Responses"?

@laymonage laymonage force-pushed the ticket_30294 branch 2 times, most recently from aea9dd1 to 315ec5f Compare Mar 28, 2019
@laymonage
Copy link
Contributor Author

@laymonage laymonage commented Mar 28, 2019

Does this look good @carltongibson?

@@ -188,7 +188,7 @@ Models
Requests and Responses
~~~~~~~~~~~~~~~~~~~~~~

* ...
* Added :class:`memoryview` compatibility to :class:`~django.http.HttpResponse`.
Copy link
Member

@timgraham timgraham Mar 28, 2019

Choose a reason for hiding this comment

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

* Allowed :class:`~django.http.HttpResponse` to be initialized with
  :class:`memoryview`.

The documentation for HttpResponse should also be updated.

@@ -366,6 +366,10 @@ def test_non_string_content(self):
r.content = 12345
self.assertEqual(r.content, b'12345')

def test_memoryview_content(self):
r = HttpResponse(memoryview(b"a memoryview object"))
Copy link
Member

@timgraham timgraham Mar 28, 2019

Choose a reason for hiding this comment

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

Use single quotes (unless a string contains a single quote) as described in Python coding style.

Copy link
Contributor Author

@laymonage laymonage Mar 28, 2019

Choose a reason for hiding this comment

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

Done.

@laymonage laymonage force-pushed the ticket_30294 branch 2 times, most recently from 3c30471 to 67b19ca Compare Mar 28, 2019

>>> response = HttpResponse(b"Here's the text in bytes.")
>>> response = HttpResponse(memoryview(b"Here's a memoryview object."))

Copy link
Contributor Author

@laymonage laymonage Mar 28, 2019

Choose a reason for hiding this comment

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

How about this @timgraham?

Copy link
Member

@timgraham timgraham Mar 28, 2019

Choose a reason for hiding this comment

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

Please rebase your branch to pickup the changes from e449c3a. A .. versionchanged:: 3.0 annotation should also be added.


>>> from django.http import HttpResponse
>>> response = HttpResponse("Here's the text of the Web page.")
>>> response = HttpResponse("Text only, please.", content_type="text/plain")
>>> response = HttpResponse(b'Bytestrings are also accepted.')
>>> response = HttpResponse(memoryview(b'Memoryview objects as well.'))
Copy link
Contributor Author

@laymonage laymonage Mar 29, 2019

Choose a reason for hiding this comment

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

@timgraham timgraham merged commit 9aa56cb into django:master Mar 29, 2019
19 checks passed
@laymonage
Copy link
Contributor Author

@laymonage laymonage commented Mar 29, 2019

I wasn't so sure about the versionchanged annotation, so I didn't put it there yet. Thanks for the example!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants