Thanks everyone for the reviews. I've pushed a new commit addressing most of the issues raised: https://github.com/django/deps/commit/62b0ee351727cb0e7ef41ba6dd2f3f7280a219de
More comments and replies (to various people in the thread) below: On 01/08/2016 09:01 AM, Florian Apolloner wrote: > PR1 (allows changing the middlewares during the middleware run): > https://github.com/django/django/pull/5591 > PR2 (static middleware chain, seems simpler and probably the way to > go): https://github.com/django/django/pull/5949/ Yes, the latter is what Pyramid does and what I had in mind; in fact, its performance benefits are one of the justifications given in the DEP for why to use the factory approach rather than simple functions. I don't think "changing middlewares per request" is a use case that's worth the performance implications (and it's not a use case we currently support anyway). Thanks for the super-quick turnaround on implementation! On 01/08/2016 10:35 AM, Aymeric Augustin wrote: > The only (and minor) concern I have is about allowing function-based > middleware factories to return None. > > In the spirit of “there should be only one way to do it”, I would > require raising MiddlewareNotUsed explicitly to disable a middleware. > A middleware factory that returns None would cause an > ImproperlyConfigured exception. Otherwise middleware could be skipped > by mistake, if the developer forgets to return the middleware from > the factory. This is especially a concern for production-only > middleware that developers can’t run locally. [snip] Yes, you and Ryan are absolutely right, allowing `None` was a mistake and I've removed it from the spec. For function-based middleware, there's also the option to simply return the ``get_response`` callable you were given, which has the same effect of "skipping yourself." That falls out naturally from the concept, so I don't see any reason to disallow it, but we don't need to mention it in the docs if we want to stick to "one way to do it." On 01/08/2016 04:51 AM, Markus Holtermann wrote: > I would, however, include the exception handling in the examples > provided in section "Specification" as that is an integral part of > middlewares, too. Good idea, I've made that change in the commit linked above. > Nitpicking, I would also name the settings variable MIDDLEWARES > (i.e.plural) as it is a list of middlewares, not just one. Well, this is a matter of some debate :-) See e.g. https://github.com/rack/rack/issues/332, where a number of people fervently argue that "middleware" is a "mass noun" along the lines of e.g. "furniture", and that it is even incorrect to say "a middleware" (much like you would never say "a furniture"); instead we should always say "a middleware component." I think those people are fighting a lost battle; the usage of "middleware" as singular is already well established in the Python world, even outside Django; people frequently talk about "a WSGI middleware." That said, my ear still prefers "middleware" as both the singular and the plural (there are such words in English) and dislikes "middlewares." So I'd prefer to stick with MIDDLEWARE and haven't changed it in the spec. But if most people think MIDDLEWARES is better, I won't stand in the way. We could also go with something like MIDDLEWARE_FACTORIES or MIDDLEWARE_PIPELINE and avoid the issue altogether :-) Carl -- 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 https://groups.google.com/group/django-developers. To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/5690020C.8090106%40oddbird.net. For more options, visit https://groups.google.com/d/optout.
signature.asc
Description: OpenPGP digital signature
