[patch] More flexible render_to_response()

[Brent Hagany]

This is the first time I've submitted a patch (anywhere), so if I've made some unbelievable gaffe, please be gentle, and I will do what I can to fix. This is just a little improvement I've been using on my own for a while - it allows you to specify a response_class kwarg when calling render_to_response, if you'd prefer to send something other than an HttpResponse.

[Brent Hagany]

Updated patch to include docs

[Eric Holscher]

Render to response is simply a shortcut function, to render to a response. Is there a reason that you can't simply just use your own render_to_customresponse function?

This does seem to provide useful functionality, and defaults back to the old behavior, so nothing should break.

[Brent Hagany]

@eric, I actually pulled this from my collection of useful little functions to submit it here. I chose it because I wanted to test the waters of contributing to Django, and I thought that this would be a nice, unobtrusive addition that remains in the spirit of the original function. Also, I remember Jacob (it might have been Adrian - don't remember clearly) mentioning django.shortcuts as needing an overhaul to be more useful in one of the DjangoCon videos.

@jacob, thanks :)

[Eric Holscher]

@bhagany: Cool. I agree that it is generally useful and thanks for the patch! I'm still trying to get a feel for what should go in core and what should be left out, it seems like i'm almost always wrong :)

Cheers.

[ccahoon]

This same thing can be done by: HttpResponse(render_to_string(...), ...), with whatever response class other than HttpResponse. Another solution, if we like the render_to_response shortcut, might be allowing the HttpResponse object itself to be passed into the shortcut. I think, though, that it isn't necessary.

[Brent Hagany]

Man, I am confused as to what 'Accepted' from a core committer means.

@ccahoon - it's true, the HttpResponse(render_to_string(..), ..) syntax is pretty much exactly what render_to_response does. I guess I am confused as to why render_to_response is there in the first place, if there's that much decision/discussion to be had around something like this? I just want a consistent, and easy way to render responses. render_to_response is easier by a hair, but I can't get the consistency I need out of it. That's the only reason for this patch.

[ccahoon]

@bhagany: render_to_response is what developers need in a huge number of cases to quickly render a template in a view. render_to_string covers many other cases.

The way that this patch provides additional functionality is a bit confusing, I think. By passing in the response class that we will be using, then passing in arguments to be provided to that response class, we have already had to use the class names and arguments we would need to just instantiate a response. Doesn't it seem like we would be better off creating the response anyway? It doesn't even seem like extra work, and that way we have the option to actually manipulate the response before it gets returned. On top of that, it just makes me feel uncomfortable (code smell?) to provide the class name and arguments for it in the same argument list -- it does not seem clean.

I understand the desire for the shortcut, but I think it comes at the expense of the simplicity of the render_to_response shortcut. I think a better option might be to add another one, which has greater flexibility and is a bit more intuitive. But it could be that I am being too much of a stickler.

Maybe we should bring up the discussion on the Django Developers Google Group?

[Brent Hagany]

@ccahoon - as far as it being confusing and code-smelly to pass in a class and arguments, I refer you to get_object_or_404 and get_list_or_404, which do pretty much this. render_to_response itself (well, really HttpResponse and company, render_to_response by proxy) also allows something very close to this with context_instance - you pass a context instance and what actually goes into that context as separate arguments. I just took my cue from the other shortcut functions that require this kind of flexibility. As far as I can see, if this patch is wrong in this way, then most of the existing shortcuts are also wrong.

we have already had to use the class names and arguments we would need to just instantiate a response. Doesn't it seem like we would be better off creating the response anyway? It doesn't even seem like extra work...

With render_to_response, you don't have to mess with the template loader or instantiate a Context. That's the work we're saving, and most of the reason we use render_to_response in the first place.

... and that way we have the option to actually manipulate the response before it gets returned

Why can't you do that with render_to_response? Just assign it to a var instead of returning it directly.

but I think it comes at the expense of the simplicity of the render_to_response shortcut

I strongly disagree. People can continue to use render_to_response with exactly the same amount of simplicity as they always have. It's a completely transparent, fully backwards compatible change. Or are you claiming that this patch itself is complex? I just don't see any justification for this statement at all.

I don't think that this issue alone is worth bringing up on the development list. However, I would like some clarification on what direction the shortcuts module should be taken. I referenced a comment from either Jacob or Adrian (memory fails me) above that I saw in the DjangoCon vids last year, that shortcuts needs an overhaul. I have tried to provide some very simple, useful extensions to a single shortcut, but I have been met with either silence or a resistance that doesn't make sense to me. Perhaps I misunderstood what I heard in the video? There seems to be an overall sentiment on the development list that things don't get into Django that people can do themselves, but this sentiment runs directly contrary to the raison d'etre of having a shortcuts module at all. So, I would like to know, specifically about the shortcuts module, are we going to provide useful and flexible functionality, or are we going to tell people that they can do it themselves? Because if it's the latter, nothing will get into shortcuts ever again, and we shouldn't be saying that it needs work. Shortcuts are the very definition of stuff you can do yourself.

Most of this is not directed at you, Chris. The mixed signals are just confusing and frustrating.

[ccahoon]

@bhagany: To start, I think you're right that we should overhaul the shortcuts. I would like to help with it, and sorry I provided what seems like kind of strong resistance. I imagine a lot of it came from code-smell and the effects of trying to really minimize the changes I have been making on my branch in hopes that I can get a lot of the work put into trunk -- nothing unnecessary (at least when I remember). So now, to sorting out how we want to go about improving this shortcut.

I see now why the route you took made sense, in the context of looking at the other shortcuts, but there are two reasons why I continue to disagree with the implementation:

1) Those two shortcuts you mentioned are a pretty different situation from this one, because they essentially wrap the call so that we can 404 if it doesn't work. And _get_queryset, which doesn't pass arguments, only exists because the classes you would want to get a QuerySet from behave differently. This shortcut (render_to_response), though, just does some dirty work for us and maybe allows some customization.

2) There is an easier way of doing the same thing (which doesn't smell to me). Take a look at #10588. It solves the same problem, but in a manner that is more appropriate to how HttpResponse and its children work. The different response classes, except for Redirect, NotAllowed, and SendFile (on my branch), don't actually behave any differently except for the status codes (which can be changed already through a kwarg). And those three that I mentioned do not have need for template rendering. Passing in a class is less direct than necessary.

So to improve this shortcut, I'd suggest that we perhaps allow it to deliver all of the possible kwargs for an HttpResponse. Mimetype is actually sort of deprecated -- it's really content-type that we want. So say we allow content_type, status(code), and request (assuming my charset/codec changes get merged with trunk). With your docs and the 10588 change and tests (with the additional kwargs), this would be a useful change. Do you degree, since the other response classes that need templates are essentially vestigial, that if people really need to use them, they can deal with a context and template loader?

Especially now that the project is moving towards 1.2, I think a nice long look at "overhauling," and hopefully augmenting the shortcuts, is a great idea. Within that context, I think the change makes a lot of sense.

[Malcolm Tredinnick]

I'm far from convinced that any "overhauling" is needed. render_to_response() works perfectly for what is intended. The feature requested in this ticket can be done in a single line of code without any modifications. Adding yet more complexity to the shortcut stops it being a simple shortcut. Moving to design decision needed. I'm pretty close to just wontfix on this, since it's unnecessary, but if another core maintainer speaks up in favour of it, they get to adopt it.

[Brent Hagany]

@ccahoon - The approach in #10588 is completely acceptable to me. If you feel better about that one, feel free to go with it instead, without any complaint from me. However, it looks like you're getting some pushback from your mentor :)

@mtredinnick - I was actually thinking of you when I was talking about getting resistance, even though you hadn't spoken up on this ticket yet. It just seems like the kind of thing you don't like. Is it your opinion then that shortcuts should stay how they are pretty much indefinitely? Also, out of curiosity, was it you disagreeing with Simon at DjangoCon off mic here: ​http://www.youtube.com/watch?v=M1Qr9rSBGBE&feature=PlayList&p=D415FAF806EC47A1&index=2#t=37m10s ?

I disagree with your position, obviously. Respectfully, my disagreements with your position are:

1. You say it "works perfectly for what is intended", but I am forced to wonder what you mean. From my point of view, render_to_response is intended to avoid having to import a template loader, a context class, and a response class all the time. This patch and #10588 simply increase the efficacy of render_to_response with regard to that intent. I don't understand how it can be claimed that this idea is beyond the scope of a shortcut like this.

2. "can be done in a single line of code" - not if you count imports. A niggly point, but still. Disliking the number of things I need to import into a views.py is one of the major reasons I like the idea of shortcuts.

3. "Adding yet more complexity to the shortcut stops it being a simple shortcut" - You seem to imply that render_to_response is already complex with "yet", and then call it simple, which I find confusing. Further, I dispute that either the original function, this patch, or #10588 can be rightfully regarded as complex in any sense.

4. "since it's unnecessary" - This is not a disagreement per se, but of course this patch is unnecessary. A shortcut by its very nature is unneccessary. This is the reason I am calling for a discussion on the direction of the shortcuts module as a whole, since anything that lands there will run contrary to the usual "you can do that yourself" ethos that (I think rightfully, most of the time) predominates responses to feature requests.

I will bring up the larger issue on the development list.

[Chris Beaven]

I'm also not convinced this is a common enough case to add extra complexity to a 'shortcut'.

There are other (reasonably compact) ways to still achieve this.