Hello and thanks for the review, First I will say that most of this pull request is a port of https://github.com/django/django/pull/1037 which I ported more or less blindly.
On Sunday, 23 August 2015 19:18:20 UTC+2, Aymeric Augustin wrote: > > > 1) About the general API design > > This patch adds 7 new “stream” public APIs that duplicate the current > “render” > APIs. Adding a stream=False argument to the current APIs would be a more > lightweight alternative. Passing stream=True would enable the streaming > behavior. Was this alternative considered and if so, why was it rejected? > > I'm not sure it was considered, but I don't think it would really be less intrusive. The documentation would be better, but the code would be less readable with a lot more conditions in the rendering logic (as opposed to keeping the same logic in stream and joining the streams in render). > I think it would make the patch less intrusive and keep the documentation > more > readable. Specifically I'm concerned about making several parts of the > documentation twice longer for a relatively uncommon need. > > It's unclear to me why TemplateView gets a StreamingTemplateView sibling > while > other generic class base views that inherit TemplateResponseMixin don't. > All > GCBVs except View and RedirectView could get a Streaming version. Even if > the > initial patch doesn't do it, someone will do it eventually, which amounts > to > 13 additional classes. > > As I said, it was done on the original PR so I did it too. Maybe putting it as an example in the doc would be enough, so users know how to use streaming templates in GCBVs. > I'm -1 on the concept of duplicating every Django API that deals with HTTP > responses. That design doesn't strike a good balance between simplicity for > newcomers and power for advanced users. > > 2) About third-party template engines > > How should third-party template engines that don't support streaming > rendering > behave? Neither the code nor the release notes provide any guidance on this > issue. I suggest to add a stream() method that raises NotImplementedError > to > the base class for template backends. > > The Template class in the backends doesn't inherits from another class though, unless I'm missing something ? > The current patch implements a stream() method that doesn't actually > stream in > the DummyBackend and a boilerplate render() method that concatenates a > list of > one element. This is bad, all the more since the dummy backend is intended > to > be a template for third-party engines. > > Should the NotImplemented be in the DummyBackend stream method then ? > All template backends have the same copy-pasted render() method. Perhaps > it's > a symptom of a problem with the API. Perhaps it should just be pulled to > the > base class. > > So we should have a base class for backends' Template class ? > 3) About performance > > I think benchmarks are required: > > a. to ensure that this change doesn't degrade performance for the > traditional > rendering mode > > Yann Malet's recent bug report shows that even modest performance > regressions > in template rendering can be a real problem for users. > > b. to assess the performance of streaming rendering which could be > unexpectedly slow on a realistic network (try over 3G tethering) > > I'm bringing this up because streaming rendering will often yield many > small > chunks. If each of these ends up in its own TCP packet -- which is going to > happen unless the application server provides additional buffering -- I > expect > poor performance. If that's confirmed, it should be mentioned in the docs. > Streaming rendering will still be useful for rendering gigantic amounts of > data. But it isn't the performance optimization it may look like. > > I agree, do you know what tools could I use to emulate 3G ? > -- > Aymeric. > > > > On 19 août 2015, at 00:18, Tim Graham <timog...@gmail.com <javascript:>> > wrote: > > I'd like to ask for a high-level API review of some proposed streaming API > additions (I have already given the patch a couple of detailed reviews, > but > other eyes would be welcome on the details as well). > > Summary: > > * django.views.generic.base.StreamingTemplateView to stream a template > rather than render it. > > * A Template.stream() method, a django.template.loader.stream() function, > and django.shortcuts.stream_to_response() and > django.shortcuts.stream() shortcuts. > > * django.template.response.StreamingTemplateResponse and > django.template.response.SimpleStreamingTemplateResponse classes to > handle streaming of templates. > > Pull request: > https://github.com/django/django/pull/4783 > > Thanks! > > -- > 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-develop...@googlegroups.com <javascript:>. > To post to this group, send email to django-d...@googlegroups.com > <javascript:>. > 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/772cae79-1941-4a6a-94a8-b82d5e767aa1%40googlegroups.com > > <https://groups.google.com/d/msgid/django-developers/772cae79-1941-4a6a-94a8-b82d5e767aa1%40googlegroups.com?utm_medium=email&utm_source=footer> > . > For more options, visit https://groups.google.com/d/optout. > > > -- 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/53d38f08-34cb-4473-92a0-d629335cb97f%40googlegroups.com. For more options, visit https://groups.google.com/d/optout.
