Code

Opened 5 years ago

Closed 14 months ago

#10504 closed Cleanup/optimization (fixed)

Consistency between HttpResponse* params and render_to_response

Reported by: bhagany Owned by: nobody
Component: HTTP handling Version: 1.0
Severity: Normal Keywords:
Cc: brent.hagany@…, riquito Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: yes UI/UX: no

Description

I was just browsing through some of the Django code and came across the new-ish (to me) addition of the content_type alias for the mimetype parameter to HttpResponse and company. Wondering about the rationale behind this decision, I came across ticket #3526, which may be helpful to review in evaluating this ticket.

In light of this, (and just for consistency) I thought it would probably be a good idea to allow render_to_response to pass content_type along to HttpResponse. I'll attach a simple patch that does this.

Also, this patch will conflict with another patch (#9081) I've submitted that has been accepted but not yet committed. It'll be very easy to resolve, but I'm unsure if I should take any special measures about this.

Attachments (2)

content_type_in_render_to_response.diff (1.2 KB) - added by bhagany 5 years ago.
tkt-10504.diff (3.6 KB) - added by kenth 2 years ago.

Download all attachments as: .zip

Change History (19)

Changed 5 years ago by bhagany

comment:1 Changed 5 years ago by bhagany

  • Cc brent.hagany@… added
  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 5 years ago by bhagany

  • Component changed from Uncategorized to HTTP handling

comment:3 Changed 5 years ago by jacob

  • Triage Stage changed from Unreviewed to Accepted

comment:4 Changed 5 years ago by rvanlaar

  • Needs tests set

Needs tests.

comment:5 Changed 5 years ago by bhagany

My initial reaction - really?

1) Nobody has found it necessary to write a single test for a shortcut before (unless I just don't understand where they would be)
2) This is less than a one line change
3) This ticket has been touched by a core committer and accepted, despite the absence of tests.

So, I mean, I can do this. But because of these things, I had been working under the assumption that it was not necessary. Please advise.

comment:6 Changed 5 years ago by ccahoon

  • Needs tests unset

@bhagany As I understand it, "Accepted" means that he thinks the general idea of the ticket is valid, not that the patch is accepted. If the patch was accepted, jacob would have committed the changes. I think it's a fine patch, though. I found this ticket looking to see if someone had already done the work I was about to on my GSoC branch, and there yours is. I am going to apply your changes to my SoC branch, so it will be bound to get reviewed within a month or so.

comment:7 Changed 5 years ago by ccahoon

@bhagany Actually, I will probably spend some time hopping between the two patches you've submitted and get advice from my mentor. They both look good, but the other is indeed very flexible, and I don't see any problems with it yet.

comment:8 Changed 5 years ago by bhagany

@ccahoon Sounds great! I've been following the work in your branch, and I'm happy to have saved you a teensy bit of work.

Also, re: accepted status - noted, and thanks for explaining.

comment:9 Changed 5 years ago by SmileyChris

ccahoon: have you fixed this in your branch? (if so, update the triage stage)

comment:10 Changed 3 years ago by SmileyChris

  • Severity set to Normal
  • Type set to Cleanup/optimization

comment:11 Changed 3 years ago by julien

  • Needs tests set

comment:12 Changed 2 years ago by d0ugal

  • Easy pickings set
  • UI/UX unset

Changed 2 years ago by kenth

comment:13 Changed 2 years ago by kenth

  • Needs tests unset

Added tests for new shortcut.

comment:14 Changed 22 months ago by anonymous

  • Resolution set to invalid
  • Status changed from new to closed

In [da200c5e35cd] (in response to ticket #16519 ) mimetype has been deprecated, so the patch isn't anymore needed.

comment:15 Changed 22 months ago by riquito

  • Cc riquito added

I sat the resolution to invalid erroneously as anonymous, ccing now to follow evenctual future messages

Last edited 22 months ago by riquito (previous) (diff)

comment:16 Changed 22 months ago by claudep

  • Patch needs improvement set
  • Resolution invalid deleted
  • Status changed from closed to reopened

In [da200c5e35cd], we didn't address the render_to_response inconsistency, so I think that the attached patch still makes sense. We should however introduce a deprecation path for the mimetype argument also.

comment:17 Changed 14 months ago by timo

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

Deprecating of mimetype in render_to_response completed in #19692 and 89cb771be7b53c40642872cdbedb15943bdf8e34.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.