Fixed #24139 -- Evaluate reason_phrase on demand based on status_code.

[jdufresne]

No description provided.

[shaib]

buildbot, test this please.

[MarkusH]

You don't set self._reason_phrase anywhere in here if the lookup fails. I don't think that's intended?

[collinanderson]

It makes sense to me that you would leave it unset unless self.reason_phase explicitly sets it.

[MarkusH]

Can we have that as a comment then, please.

[jdufresne]

Can we have that as a comment then, please.

Added comment as suggested. Let me know if you had something else in mind.

[jdufresne]

Rebased after the merge of #3902 and resolved conflicts.

[berkerpeksag]

:attr:reason_phrase or reason_phrase

[jdufresne]

Perhaps we can use a similar approach for status_code and status attributes, too.

Do you mind expanding on what you're requesting?

Are you saying status_code should not be a class attribute on HttpResponseBase? The other classes in this file rely on this behavior: HttpResponseRedirect, HttpResponseNotFound, etc.

Or are you saying the __init__() kwarg status should be renamed to status_code?

Sorry, but I'm not sure precisely what "status attributes" is referring to.

I'm happy to cleanup whatever needs it, I'm just not sure exactly what you're asking me to change.

I suppose one potential fix would be to fix the Content-Type header to evaluate based on the current value of self._charset. Is this along the lines of what you mean?

Version in master (if charset changes outside the constructor, the Content-Type header is wrong):

    def __init__(self, content_type=None, status=None, reason=None, charset=None):
        ...
        self._charset = charset
        if content_type is None:
            content_type = '%s; charset=%s' % (settings.DEFAULT_CONTENT_TYPE,
                                               self.charset)

[berkerpeksag]

Sorry for being unclear.

Or are you saying the init() kwarg status should be renamed to status_code?

That's what I meant to say. However, adding a new status_code keyword argument would be better for backward compatibility.

[jdufresne]

That's what I meant to say. However, adding a new status_code keyword argument would be better for backward compatibility.

Thanks. Will do.

[jdufresne]

I believe I have implemented what you had in mind. Let me know how you feel about the results.

[MarkusH]

This raises an error:

Traceback (most recent call last):
  File "/home/jenkins/workspace/pull-requests-trusty/database/mysql/label/trusty-pr/python/python2.7/tests/template_tests/test_response.py", line 265, in test_args
    'application/json', None, None, None, 504)
  File "/home/jenkins/workspace/pull-requests-trusty/database/mysql/label/trusty-pr/python/python2.7/django/template/response.py", line 201, in __init__
    RemovedInDjango20Warning, stacklevel=2)
RemovedInDjango20Warning: The current_app argument of TemplateResponse is deprecated. Set the current_app attribute of its request instead.

[timgraham]

This change seems to have gotten a bit unwieldy as far as the scope of the original ticket goes. Is there really a need to deprecate status in favor of status_code? I'd try to keep solving this ticket more minimal unless all these changes are really necessary to do together.

[jdufresne]

This change seems to have gotten a bit unwieldy as far as the scope of the original ticket goes. Is there really a need to deprecate status in favor of status_code? I'd try to keep solving this ticket more minimal unless all these changes are really necessary to do together.

I think I agree. In my opinion, there is no pressing need to deprecate status in favor of status_code (although I agree it is a nice to have for consistency). Shall I discard the 2nd commit? If still preferred I can move the 2nd commit to a different PR.

I originally went down this path at the request of @berkerpeksag. As a core contributor and Django Fellow I interpret his review as authoritative.

If there is disagreement about the 2nd commit, please let me know how to proceed.

[jdufresne]

OK, I've dropped the rename kwargs commit. If that is still desirable I can move it to another ticket. This will change will simply cover the goals of the original ticket.

[timgraham]

merged in d861f95, thanks!