Hi, sorry for the delay, On Monday, 7 September 2015 16:05:35 UTC+2, Aymeric Augustin wrote: > > Hello, > > 2015-09-07 10:00 GMT+02:00 Yann Fouillat <gaga...@gmail.com <javascript:>> > : > >> 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. >> > > As far as I can tell, this previous PR was never discussed on > django-developers. Someone threw the code on GitHub, that's all; you can't > make any assumptions about its suitability for inclusion in Django. Usually > we discuss how a feature should be implement, look for consensus, and then > submit the code ;-) > > > 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). >> > > The point of Django is to handle the painful stuff in the framework to > make our users' lives easier. If we need to write slightly more complicated > code to improve the API, that's just fine. > > Also I doubt there will be many more conditions. The higher levels will > just pass the stream keyword argument to the lower levels and eventually to > the template engine. The only conditional should be be `response_class = > StreamingHttpResponse if stream else HttpResponse`. > > I'm aware of the irony of me making this argument. I set a bad precedent > when I added StreamingHttpResponse. I didn't think of > HttpResponse(stream=True) at the time. The prospect of having 25 > StreamingFooBar classes in Django makes me realize this wasn't a great > idea. Let's learn from this mistake. (The stakes are a bit low to consider > fixing it through a deprecation path, though.) > > > 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 proposing to add a `stream = False` attribute to TemplateResponseMixin. > > I think documenting that changing the `response_class` attribute should be enough. Adding a `stream` attribute would make two attributes for selecting the response class (the stream attribute would not change anything else).
> > 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 ? >> > > I'm talking about template backends e.g. > https://github.com/django/django/blob/aaacaeb0/django/template/backends/django.py#L21 > . > > These backends don't have any render method. The render method is on the Template class: https://github.com/django/django/blob/aaacaeb0963c406c4fe6f68c6ae51f4a65878250/django/template/backends/django.py#L54 > > >> 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 ? >> > > Yes, in order to keep > https://github.com/django/django/blob/adff499e/django/template/backends/dummy.py#L17 > > a simple example of how to implement a Django template backend. > > > >> 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 ? >> > > You're mixing template backends (instantiated once per template engine > configured in settings.TEMPLATES) and templates (instanciated once per call > to render() or equivalent). > > > >> 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 agree, do you know what tools could I use to emulate 3G ? >> > > As far as I know, the canonical tools are: > > - on Linux, netem: > http://www.linuxfoundation.org/collaborate/workgroups/networking/netem > - on OS X, Network Link Conditioner: > https://developer.apple.com/library/ios/documentation/NetworkingInternetWeb/Conceptual/NetworkingOverview/WhyNetworkingIsHard/WhyNetworkingIsHard.html > > Thanks I'll take a look at them. > Best regards, > > -- > Aymeric. > -- 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/a49bcd75-dc22-45a1-afbf-0c933a1d0e0d%40googlegroups.com. For more options, visit https://groups.google.com/d/optout.
