Opened 4 years ago

Closed 3 years ago

#16519 closed Cleanup/optimization (fixed)

mimetype used with HttpResponse should be deprecated

Reported by: PaulM Owned by: yasar11732
Component: HTTP handling Version: 1.3
Severity: Normal Keywords:
Cc: yasar11732 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 yasar11732 3 years ago.
Fixed all necessary things, but didn't write tests.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 4 years ago by jezdez

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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 4 years ago by PaulM

  • Owner changed from nobody to PaulM

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 4 years ago by PaulM (previous) (diff)

comment:3 Changed 4 years ago by PaulM

  • Easy pickings set
  • Owner PaulM deleted

comment:4 Changed 4 years ago by yasar11732

  • Owner set to yasar11732
  • Status changed from new to assigned

comment:5 follow-up: Changed 4 years ago by 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?

comment:6 Changed 4 years ago by PaulM

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 4 years ago by yasar11732

  • Cc yasar11732 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 4 years ago by ramiro

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 3 years ago by yasar11732

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

comment:9 Changed 3 years ago by yasar11732

  • Has patch set
  • Patch needs improvement set
  • Triage Stage changed from Accepted to Ready 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 3 years ago by yasar11732

  • Triage Stage changed from Ready for checkin to Accepted

comment:11 Changed 3 years ago by yasar11732

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

comment:12 Changed 3 years ago by adrian

In [17223]:

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

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

In [deed192dda621f1cdc8eb7240f1e5914045402dd]:

Removed usage of mimetype kwarg of HttpResponse

Refs #16519.

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

  • Resolution set to fixed
  • Status changed from assigned to closed

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