Opened 5 years ago

Closed 5 years ago

#16519 closed Cleanup/optimization (fixed)

mimetype used with HttpResponse should be deprecated

Reported by: Paul McMillan Owned by: Yaşar Arabacı
Component: HTTP handling Version: 1.3
Severity: Normal Keywords:
Cc: Yaşar Arabacı Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: yes UI/UX: no

Description

Passing the mimetype argument when instantiating HttpResponse should raise a DeprecationWarning.

The mimetype argument has been deprecated via comment in the code for 4 years: https://code.djangoproject.com/changeset/5844/django/trunk/django/http/__init__.py

It's about time we actually raised a DeprecationWarning and remove the thing ASAP. If we swap the position of the mimetype and content_type arguments, code the relies on argument position will continue to function without raising the warning. The only code that will need to be changed is code that explicitly uses the mimetype kwarg (including a bunch of django tests).

This situation is long-lived enough to bypass the PendingDeprecationWarning.

Attachments (1)

0001-Fix-16559-Deprecate-mimetype-for-HttpResponse.patch (17.8 KB) - added by Yaşar Arabacı 5 years ago.
Fixed all necessary things, but didn't write tests.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 5 years ago by Jannis Leidel

Triage Stage: UnreviewedAccepted

I don't think it hurts to keep the backwards compatibility code for another release, so this should definitely be a PendingDeprecationWarning

comment:2 Changed 5 years ago by Paul McMillan

Owner: changed from nobody to Paul McMillan

I suppose it doesn't hurt anything to have it pending for a bit. I'll work on a patch when I get a chance.

Last edited 5 years ago by Paul McMillan (previous) (diff)

comment:3 Changed 5 years ago by Paul McMillan

Easy pickings: set
Owner: Paul McMillan deleted

comment:4 Changed 5 years ago by Yaşar Arabacı

Owner: set to Yaşar Arabacı
Status: newassigned

comment:5 Changed 5 years ago by Yaşar Arabacı

I am new to contributing, and I shoud ask if I should fix contrib packages too. I mean replacing mimetype with content_type where needed?

comment:6 Changed 5 years ago by Paul McMillan

Yes, you'll need to fix it everywhere it occurs in the Django codebase, including the tests. You'll also probably need to write some new tests. If you swap the order of mimetype and content_type in the definition, the functions that are using mimetype as a positional argument will transition more easily.

comment:7 Changed 5 years ago by Yaşar Arabacı

Cc: Yaşar Arabacı added

Ok. A patch would be available in couple of days I guess. I am excited as this will be my first actual contrib to django :)

comment:8 in reply to:  5 Changed 5 years ago by Ramiro Morales

Replying to yasar11732:

I am new to contributing, and I shoud ask if I should fix contrib packages too. I mean replacing mimetype with content_type where needed?

If you are talking about django.contrib.* then the answer is yes. Thanks for looking at this.

Changed 5 years ago by Yaşar Arabacı

Fixed all necessary things, but didn't write tests.

comment:9 Changed 5 years ago by Yaşar Arabacı

Has patch: set
Patch needs improvement: set
Triage Stage: AcceptedReady for checkin

I made a patch file where I made warning and changed mimetype kwarg, to content_type kwarg. However, I didn't write tests for it, as I am not sure what I am test for here, something like this maybe?

import warnings
from django.http import HttpResponse

with warnings.catch_warnings(record=True) as w:
    warnings.simplefilter("always")
    HttpResponse(mimetype="text/html")

    assert len(w) == 1
    assert issubclass(w[-1].category, DeprecationWarning)
    assert "deprecated" in str(w[-1].message)

comment:10 Changed 5 years ago by Yaşar Arabacı

Triage Stage: Ready for checkinAccepted

comment:11 Changed 5 years ago by Yaşar Arabacı

Sorry about marking as RFC. I didn't know I didn't supposed to do that.

comment:12 Changed 5 years ago by Adrian Holovaty

In [17223]:

Converted some of the built-in views to use content_type instead of mimetype HttpResponse argument. Refs #16519

comment:13 Changed 5 years ago by Claude Paroz <claude@…>

In [deed192dda621f1cdc8eb7240f1e5914045402dd]:

Removed usage of mimetype kwarg of HttpResponse

Refs #16519.

comment:14 Changed 5 years ago by Claude Paroz <claude@…>

Resolution: fixed
Status: assignedclosed

In [da200c5e35cdbb617572977bcbf97fae33920b2e]:

Fixed #16519 -- Deprecated mimetype kwarg of HttpResponse init

This keyword was already deprecated in the code (supported for
backwards compatibility only), but never formally deprecated.
Thanks Paul McMillan for the report and yasar11732 for the initial
patch.

Note: See TracTickets for help on using tickets.
Back to Top