Hello and thanks for the review, On Monday, 24 August 2015 15:25:11 UTC+2, Tom Christie wrote: > > Potential there to treat these as separately reviewable pull requests. > > For example - needing streaming template generation doesn't *necessarily* > imply needing streaming responses. The node-by-node rendering might mean > imply that less memory is used during the template rendering, even if the > complete response content all ends up in memory. (I haven't got a handle > from the pull request on if that'd be the case or not, and it's possible > that I've misrepresented it, and there's no benefit other than being able > to stream the output bytes) > > More generally: As presented there's lots of technical change, with very > little simple end-user presentable "why and when this is a benefit?". > > * Template.stream() > > What's the data to back this up? > Does this result in lower memory usage during the template generation, or > is there no difference aside from allowing the output to be streamed? If > there is an internal benefit then how large do those templates need to be > before that's significant? > > As said in the other review, benchmarking is necessary to determine that.
> * StreamingTemplateResponse > > Again, where's the data to back this up? We're introducing a decision > point for our users without giving them any basis on which to make that > decision. At what point would I likely want to use this, and what > real-world cases for our users are driving this? (For really large CSV > responses are templates a good idea in any case?) > > I also don't really understand from the documentation how the behavior for > SimpleStreamingTemplateResponse and StreamingTemplateResponse differs - > "does not handle any rendering attributes/methods (including callbacks)" - > I understand what the callbacks refer to there, but what does the rest of > that mean? > > The methods defined by SimpleTemplateResponse (https://github.com/Gagaro/django/blob/ticket_13910/django/template/response.py#L112) are not available to SimpleStreamingTemplateResponse. > * django.views.generic.base.StreamingTemplateView > > Unclear to me that the decision point this view introduces to users is > worth the benefit it provides. > Writing a view that streams a simple template is just about as simple a > view as is possible to write in Django - would it be better if we simply > took an informed decision on which of TemplateView / StreamingTemplateView > is likely to be a better default on behalf of our users? > > Also lukewarm on introducing a new GCBV that actually just requires a > one-line addition to an existing GCBV. Should we prefer to instead document > the usage of `response_class` more fully, rather than extending the API > surface area? > > As I said in the other answer, I did port this blindly from the old PR. It could be better off in the documentation instead. > Apologies for jumping in with the criticisms - intended positively! > Understand the intent behind this, but a bit wary of changes that come > across as purely technical with very little in the way of concrete > demonstration of benefit. Think it'd be good to see it approached from a > perspective of "what are we actually recommending our users do here?" > > Thanks for the criticisms, it helps going forward :). > Cheers & thanks to everyone for their hard work! > -- You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group. To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+unsubscr...@googlegroups.com. To post to this group, send email to django-developers@googlegroups.com. Visit this group at http://groups.google.com/group/django-developers. To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/c55b8260-7b82-446b-ac97-7da89582154d%40googlegroups.com. For more options, visit https://groups.google.com/d/optout.
