On Friday 28 May 2010 09:03:02 David Larlet wrote: > > This will solve the thread safety issue, and doesn't require > > another import for a decorator, as Jari pointed out. > > > > We could decide whether to apply decorators inside as_view() or > > __call__(). The argument for putting it in 'as_view' is that > > __call__ does some processing that you might want to skip > > entirely (e.g. the get_object() call - if we applied a > > 'cache_page' decorator, we wouldn't want the get_object() call > > to be made at all if there was a cache hit). > > We discussed that point with Jacob and it looks like it's easier to > modify urlresolvers to create a new instance of the class when we > detect it's a class, this should solve the thread safety issue. Do > you see any drawback to this approach?
I'm sure that will work. My main reluctance is using isinstance() when it isn't necessary, and having to change the definition of what a 'view' is when it isn't strictly necessary. (A view is no longer simply "a callable that takes a request and returns a response"). I guess this is partly about aesthetics, and I know practicality beats purity, but: There might also be the following practical disadvantages when it comes to the API we encourage. If the recommended usage is to do: urlpatterns = ... (r'something/', MyClassView) ... then it would be easy to accidentally do `MyClassView()`, and it would appear to work fine, until you later get thread safety issues. Also, if you want to pass arguments to the constructor, you would most naturally do `MyClassView(somearg=something)`, and again it would appear to work. If, however, we encouraged `MyClassView.as_view()` from the start, we can cope with constructor arguments more easily - it changes to `MyClassView.as_view(somearg=something)`. (Either way, it would still be possible to pass in MyClassView() and get thread safety problems, but it's about making mistakes less natural). Of course, if we want to use dotted path syntax in URLconfs - "path.to.MyClassView" - rather than imports, then we would *need* to have the isinstance() check to be able to cope with it at all. My response to that is: my preference would be that the use of class based views should remain an implementation detail, and as far as the URLconf is concerned, views.py should be exporting a ready-to-use callable not the class. (The class is exported for the purpose of other people subclassing, or passing their own constructor arguments, not for direct use in the URLconf). There is also the issue of what to do when you need to add a decorator to someone else's view function. As a developer, I shouldn't have to know that class based views even *exist*, let alone how to use them, if I'm just a client - I should *always* be able to say: from someapp.views import some_view_function my_view_function = login_required(some_view_function) But if someapp.views has only exported SomeClassView and not some_view_function, I now have to do something different. Given that, for many Django apps, the view functions are part of the public API that needs to be exported (i.e. hooked into people's URLconfs or wrapped for re-use), I think it would be good to encourage practices which will lead to stable and consistent APIs, and so *not* allow or encourage classes to be used directly in the URLconf. Regards, Luke -- "Oh, look. I appear to be lying at the bottom of a very deep, dark hole. That seems a familiar concept. What does it remind me of? Ah, I remember. Life." (Marvin the paranoid android) Luke Plant || http://lukeplant.me.uk/ -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
