> 3) Add the decorators when you call 'instantiator' (which, by the
> way, I would rather call 'make_view' or something more specific).
> You would then have, *in views.py*:
>
> 3a) my_view = some_decorator(make_view(MyViewClass))
>
> or possibly (though I don't think this adds much):
>
> 3b) my_view = make_view(MyViewClass,
> decorators=[some_decorator])
>
I had some more thoughts about this. One problem with applying the
decorators after is if someone is re-using the class (e.g.
subclassing), but forgets to apply some *necessary* decorators when
creating the view from it. One work-around is that 'make_view' checks
the class itself for a 'decorators' attribute, and applies them:
def make_view(viewclass):
def view(request, *args, **kwargs):
obj = viewclass()
return obj(request, *args, **kwargs)
for f in getattr(viewclass, 'decorators', []):
view = f(view)
return view
This would make it harder to forget, but would still allow subclasses
to adjust the decorators that are used by default. It won't help in
the case where people use the class directly in the URLconf, but I
don't think that should be encouraged anyway.
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/
We're working on this with Ben during djangocon.eu sprint [1]. Any comment appreciated, still a work in progress though.
Regards,
David
[1] http://github.com/bfirsh/django-class-based-views
Looks cool. I think Jari's idea of a class method to instantiate the
classes would be a great pattern to establish - something like an
'as_view' classmethod on the base class which returns a view function
that uses the class:
@classmethod
def as_view(cls, *initargs, **initkwargs):
def view(*args, **kwargs):
obj = cls(*initargs, **initkwargs)
return obj(*args, **kwargs)
return view
The developer can choose whether to write 'MyClassView.as_view()'
directly into the URLconf, or put a line into views.py like 'my_view =
MyClassView.as_view()'
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).
On that point, I think 'GenericView' should be split into 'ClassView'
and 'GenericView'. The get_object() call is probably not a good thing
to have on a base class.
Regards,
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?
That way we can declare decorators via a decorators' list [1] or directly in the class [2]. I'm not fond of the second way, decorating a decorator, but we can create decorators dedicated to class based views easily that way.
>
> On that point, I think 'GenericView' should be split into 'ClassView'
> and 'GenericView'. The get_object() call is probably not a good thing
> to have on a base class.
Agreed, I'll make that change.
Thanks,
David
[1] http://github.com/bfirsh/django-class-based-views/blob/master/class_based_views/tests/views.py#L24
[2] http://github.com/bfirsh/django-class-based-views/blob/master/class_based_views/tests/views.py#L43
> > 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.
We're currently working towards URLpatterns of the form::
('...', SomeView)
i.e. passing a class directly, which would be instantiated implicitly
then called.
The reason we're trying it this way is thread safety: if a view is a
class, then people *will* store state on self. So if we have single
instances in views like we do now, then we're giving folks enough rope
to hang themselves. By having each request get a fresh instance we
avoid that problem.
The other alternative we discussed was having the base view class do
something with threadlocals to make sure state saved on self is
threadsafe. I vetoed that because I hate threadlocals :)
On Fri, May 28, 2010 at 3:41 PM, Luke Plant <L.Pla...@cantab.net> wrote:
> 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 *am* a bit worried by this -- I'm not sure redefining the definition
of "view" is such a good idea. "A view is a callable or a class with
callable instances" isn't nearly as simple and nice a recommendation.
> 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)`.
My only real objection here is that `as_view` is just a bunch of boilerplate::
urlpatterns = patterns('',
('one/', SomeView.as_view()),
('two/', SomeOtherView.as_view()),
('three', YourView.as_view())
)
I just really don't like the way that looks.
As for accepting extra __init__ arguments: I specifically *don't* want
to allow this (at least easily) because I want to encourage
subclassing as the "correct" way of extending views. Yes, this means a
slight loss of flexibility, but it also means that we're forcing
people into a more extensible and fuure-proof way of coding, and
that's a good thing.
> (Either way, it would still be possible to pass in MyClassView() and
> get thread safety problems, but it's about making mistakes less
> natural).
... and either way it's pretty easy to issue a warning if you've
subclassed ``django.views.View`` (my proposed name for the view base
class) and re-used it for multiple requests.
> There is also the issue of what to do when you need to add a decorator
> to someone else's view function.
Again, I want to encourge "subclass it" as the correct answer here.
> 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.
I think we're in agreement here -- we're both trying to avoid view-ish
stuff in the URLconfs (a la the old way we used to do generic views
with dicts in urls.py).
Having played with it for a few years now, though, I'm finding
subclassing is really the best way to make this happen.
So reusable apps instead of exporting functional views export
class-based ones; users who want to re-use them import, subclass, and
extend.
Jacob
Storing state on self makes things a heck of a lot easier. We are going to create a "View" and a "ResourceView". A View just renders a template, but a ResourceView has some kind of resource attached to it (model instance, QuerySet, whatever). On View, this would be the get_context() signature (where args and kwargs are from the URL):
def get_context(self, request, *args, **kwargs):
....
When we create ResourceView, we need to pass the resource in here somehow, interfering with args and kwargs:
def get_context(self, request, resource, *args, **kwargs):
....
We would have to redefine all the methods which require the resource. It seems neater to store it on self.
We have solved accidentally instantiating the view by raising an exception if a View instance is called twice. Of course, people who understand the threading issues are free to create their own class-based views.
I like this method because of its transparency. I fear having to explain as_view() in the documentation. Thread problems are hard to understand.
Ben
> My only real objection here is that `as_view` is just a bunch of
> boilerplate::
>
> urlpatterns = patterns('',
> ('one/', SomeView.as_view()),
> ('two/', SomeOtherView.as_view()),
> ('three', YourView.as_view())
> )
>
> I just really don't like the way that looks.
Agreed. I also think that if you have a mixture of normal views and
class based view, this is ugly:
urlpatterns = patterns('app.views',
('one/', 'some_view_function',
('two/', SomeOtherView),
('three/', YourView)
)
and it would be nicer to spell it:
urlpatterns = patterns('app.views',
('one/', 'some_view_function'),
('two/', 'some_other_view'),
('three/', 'your_view')
)
and have these additional lines in 'app.views':
some_other_view = SomeOtherView.as_view()
your_view = YourView.as_view()
I know that is just moving the boilerplate around, but it is giving a
consistent interface. If some code in a re-usable app moves from
normal views to class based views, they will have to do something like
this *anyway* for backwards compatibility.
But I can see that if subclassing is the default way of re-using, then
exporting these functions gives multiple options about how they should
be re-used, which you are wanting to avoid.
> > There is also the issue of what to do when you need to add a
> > decorator to someone else's view function.
>
> Again, I want to encourge "subclass it" as the correct answer here.
In that case, I guess it would be good to make this hard to do wrong,
by providing helpers that add the decorator to the right end of the
list of decorators.
But don't worry, we've changed our minds again! If __call__() creates a copy of self and calls methods on the copy, as far as I can see it solves all our problems. No boilerplate, and it's really hard to make it unsafe thread-wise.
An example of how it could work:
http://github.com/bfirsh/django-class-based-views/blob/master/class_based_views/base.py
Thanks
Ben
You mean:
class View(HttpResponse):
...
One reason against this is it makes it harder to re-use other views
from within the View. You are forced to mutate the 'self' instance of
HttpResponse (or else use some nasty hack), rather than being able to
simply return the HttpResponse that might be returned from a utility
function or a sub-view.
This is an interesting approach. The only downside I can think of at
the moment is that implementing __new__() like that really violates
expectations - Python programmers have a fairly strong expectation
that if you do 'x = SomeClass()', where SomeClass is defined as a
class, then x will be an instance of SomeClass.
> On Tuesday 01 June 2010 11:43:30 henning....@gmail.com wrote:
>> On May 30, 7:24 am, Waldemar Kornewald <wkornew...@gmail.com> wrote:
>>> Maybe I missed something, but why don't you use __new__ instead
>>> of copying the instance?
>>
>> Here is an example where I used __new__ for class based views in my
>> project:
>> http://djangosnippets.org/snippets/2046/
>> No __call__ or as_view is needed with this approach.
>> I can easily replace a function with a class without changing the
>> url configuration.
>
> This is an interesting approach. The only downside I can think of at
> the moment is that implementing __new__() like that really violates
> expectations - Python programmers have a fairly strong expectation
> that if you do 'x = SomeClass()', where SomeClass is defined as a
> class, then x will be an instance of SomeClass.
Yeah, this idea came up at the sprints, but it's a little too magic for my tastes.
Ben
Hi :-)
I'm not really sure about that approach. As with __new__, __call__
comes (at least for me) with a bunch of expectations. For me it
executes and operates on one single instance of an object. Sure, this
could be interpreted as using the instance as a factory for usually
you'd use class methods for something like that, but still.
On the other hand I can definitely see the merit in it since you could
then easily programmatically modify the "view" factory by just
modifying "yet another object".
-- Horst
One advantage of using __call__ over __new__ is passing arguments to __init__. That's handy for lazily configuring views:
(r'', DetailView(queryset=Thing.objects.all())
Ben
I thought we said we really wanted to encourage using subclassing to
solve these problems (providing many types of args in urls.py can
cause problems).
Alex
--
"I disapprove of what you say, but I will defend to the death your
right to say it." -- Voltaire
"The people's good is the highest law." -- Cicero
"Code can always be simpler than you think, but never as simple as you
want" -- Me
Why can't you use this instead:
(r'', 'views.DetailView', {'queryset': Thing.object.all()})
Bye,
Waldemar Kornewald
> > One reason against this is it makes it harder to re-use other
> > views from within the View.
>
> I don't really understand what you mean. Do you mean re-using
> 'good' old function-views?
Yes, or new ones, or any callable that returns an HttpResponse that I
want to return. It is quite a common pattern to have one view wrap
another or *several* others, and also to use standard response objects
in some utility function or 'sub view'. e.g.
def my_view(request, *args):
if some_condition():
return some_view(request, *args)
elif other_condition():
return some_standard_response(request.user)
else:
...
return render_to_response(...)
def some_standard_response(user)
return HttpResponseRedirect(...)
This would be massively uglified, at best, if we make View a subclass
of HttpResponse.
Regards,
Uhm... guys,
Maybe something simpler?
---- 8< ----
from threading import local, Thread
class View(object):
_child = local()
def __new__(cls, *args, **kwargs):
existing = hasattr(cls._child, 'instance')
if not existing:
print 'first time in this thread'
cls._child.instance = super(View, cls).__new__(cls, *args, **kwargs)
return cls._child.instance
def __call__(self, foo):
print 'call', id(self), foo
return 'bar'
test = View()
test(1)
test = View()
test(2)
test = View()
test(3)
class TestThread(Thread):
def run(self):
test = View()
test(4)
t = TestThread()
t.setDaemon(True)
t.start()
t.join()
test = View()
test(5)
---- 8< ----
Try it now, tests itself.
--
Patryk Zawadzki
Actually you might want to have a couple of instances with different
__init__ params:
---- 8< ----
from threading import local, Thread
class View(object):
_child = local()
def __new__(cls, *args, **kwargs):
if not hasattr(cls._child, 'dict'):
cls._child.dict = {}
param_hash = (args, tuple(kwargs.items()))
existing = cls._child.dict.get(param_hash, None)
if not existing:
print 'first time in this thread with args', param_hash
cls._child.dict[param_hash] = super(View, cls).__new__(cls)
return cls._child.dict[param_hash]
def __call__(self, foo):
print 'call', id(self), foo
return 'bar'
--
Patryk Zawadzki
The supposed View class is a pure utility class that is close to a
module. It does not represent a state in an application, it takes
input and returns output, none of it should outlive a "__call__" call.
Therefore I see no reason for a proper implementation to not be
thread-safe. Just treat "self" as immutable past "__init__". In other
words, don't treat "self" as a junkyard for temporary variables.
This is WRONG:
class View(object):
def greet(self):
return HttpResponse('Hi %s' % self.request.POST.get('hello', ''))
def __call__(self, request):
self.request = request
return greet(self)
This is CORRECT and not any harder to design/implement:
class View(object):
def greet(self, request):
return HttpResponse('Hi %s' % request.POST.get('hello', ''))
def __call__(self, request):
return greet(self, request)
I really don't think we should add anti-moron filters at the framework
level past documenting "taking care to ensire immutability of view
instances".
A true moron will always outsmart us with creativity. It's perfectly
possible to write thread-unsafe code without using classes:
req = None
def greet(self, request):
global req
req = request
# ...
return HttpResponse('Hi %s' % req.POST.get('hello', ''))
or even:
def greet(self, request):
greet.foo = request
# ...
return HttpResponse('Hi %s' % greet.foo.POST.get('hello', ''))
:)
--
Patryk Zawadzki
On Tue, Jun 15, 2010 at 3:38 PM, Patryk Zawadzki <pat...@pld-linux.org> wrote:
> I really don't think we should add anti-moron filters at the framework
> level past documenting "taking care to ensire immutability of view
> instances".
This came out a bit harsh. What I mean is: teach users how to write
thread-safe views (classes or not) and ignore those who refuse to do
it. They either know what they're doing or there is really no point in
forcing the help upon them.
--
Patryk Zawadzki
--
Patryk Zawadzki
I consider using self to store temporary variables a bad design
practice. You wouldn't do that in C++ or Java. Having said that I
might be biased.
If you really want to do such hacks, use threading.local instead of
writing to self directly:
class View(object):
storage = threading.local()
def __call__(self, request, id):
self.storage.request = request
# ...
Cloning objects on each call to a method is a really really really
really bad idea and puts unnecessary stress on the garbage collector
(actually if any of the methods are careless enough to pass self to an
external function, it might result in a steady leak of non-freeable
memory).
--
Patryk Zawadzki
The request is passed round so methods look like views to decorators. Magically dropping it for decorators seems a bit scary. :/
Ben
Is there any reason why all of you think not passing request around is a gain?
Would you recommend anyone to do:
---- 8< ----
request = None
def my_view():
global request
return HttpResponse('Hi, %s' % (request.user.get_full_name(), ))
def my_other_view():
globl request
return HttpResponse('Hello again, %s' % (request.user.get_full_name(), ))
# ...
---- 8< ----
As I seriously see no difference between this and pushing stuff to self.request.
Request objects are esoteric. They are only meaningful during that
short period of time you spend in the __call__ method. Once you return
a HttpResponse, request is ready to retire. Storing them at the level
objects grants them potentially eternal life for no reason.
Consider this:
---- 8< ----
class AwesomeAdd(object):
def __call__(self, number1, nubmer2):
self.number1 = number1
self.number2 = number2
return self.do_the_math()
def do_the_math(self):
# OMG BBQ, we cleverly avoided passing params around!!1!one
return self.number1 + self.number2
aa = AwesomeAdd()
foo = aa(3, 5)
---- 8< ----
Doesn't that scream "WTF"? :)
--
Patryk Zawadzki
Would you seriously recommend people to extend "int" when they want to
implement multiplication? :)
--
Patryk Zawadzki
The __new__-based approach is just lovely. :) Really, it's much more
elegant than copying self and it's very convenient to be able to
access self.request and the configuration options from anywhere. This
solves a lot of problems when you want to customize a view's behavior.
We really missed this when we used Django's class-based Feed view.
Some minor issues:
The __init__ function isn't called with *args, **kwargs (at least, I
think this was an oversight).
FormView.POST should probably use self.get_content like View.GET does
by default.
Should there be different methods for each request.method, at all?
Often form POST handling code can reuse the GET part if form
validation fails. By splitting everything up in two methods you blow
up the code unnecessarily. BTW, I think the current code would
unnecessarily instantiate the form two times if the form doesn't
validate.
Also, _load_config_values should guarantee that you don't pass
unsupported arguments. This should also work with inheritance.
Bye,
Waldemar Kornewald
Please take a deeper look at his code. He doesn't use __init__. He
uses __new__, so each request gets his own View instance.
Instead of
aa = AwesomeAdd()
foo = aa(3, 5)
the __new__-based approach allows to do this:
foo = AwesomeAdd(3, 5)
IOW, the "constructor" directly returns an HttpResponse (foo) instead
of an AwesomeAdd instance.
Bye,
Waldemar Kornewald
There is no difference between using __init__ and __new__ as long as
you treat the instance as a junkyard for temporary variables. If done
properly, class-based views do _not_ need a separate instance for each
request.
Here's an example of a thread-safe view that works happily with just
one instance:
---- 8< ----
class MyView(object):
def __init__(self, queryset, template='bar.html'):
self.qs = queryset
self.template = template
def __call__(self, request, id):
instance = get_object_or_404(self.qs, id=id)
return direct_to_template(request, self.template, {'object': instance})
# ...
url(r'^foo/(?P<id>\d+)/', MyView(queryset=Foo.objects.all(),
template='foo.html')),
---- 8< ----
--
Patryk Zawadzki
This is not at all thread-safe because all requests share the same
view instance and thus the same state. Shared state is not thread-safe
(unless you introduce some locking mechanism which is not an option
here). Thread-safety can only be guaranteed if every thread has its
own state. In other words, every request needs his own View instance.
Bye,
Waldemar Kornewald
A few points:
1) With inheritance, you should be able to say Derived is-a Base, eg
'a Car is-a Vehicle'. A view is NOT a response, however you wrap it
up. A view is a generator that when called with a request, results in
a response, whether it is a class based view, or a function based
view. Does a view have headers or a mime type?
Perhaps you need to start using different names for these classes,
because inferring that 'a view is-a response' just sounds stupid.
2) I extend HttpResponse. In lots of places.
(more to the __new__ approach)
3) One of the purposes (as far as I can see) of having the view as a
class is to allow non volatile application data to be instantiated
outside of the request-response lifecycle.
In the proposed approach, each request is effectively given its own
cloned view object with which to generate the response. Surely this
means this use case is no longer achievable?
If it is, how? (I'm probably missing something!) If it isn't, then
what _is_ the use case for class based views?
Cheers
Tom
I'd like to add here that we already discussed the biggest
disadvantage of deriving from HttpResponse:
With this approach you can't easily pass the request to some other view:
def main_view(request, ...):
if ...:
return sub_view(request, ...)
else:
return other_sub_view(request, ...)
This is a common pattern and it can occur somewhere deep within the
view code. The HttpResponse approach doesn't provide an easy solution.
It doesn't even provide an easy solution for returning
HttpResponseRedirect or other HttpResponse subclasses, as you already
identified.
> (more to the __new__ approach)
> 3) One of the purposes (as far as I can see) of having the view as a
> class is to allow non volatile application data to be instantiated
> outside of the request-response lifecycle.
> In the proposed approach, each request is effectively given its own
> cloned view object with which to generate the response. Surely this
> means this use case is no longer achievable?
> If it is, how? (I'm probably missing something!) If it isn't, then
> what _is_ the use case for class based views?
It's still possible in exactly the same way as you do it today:
mydata = ...
urlpatterns('', url('...', 'MyView', {'outside_of_lifecycle': mydata}))
Alternatively, you can derive from MyView:
class SubMyView(MyView):
ouside_of_lifecycle = mydata
However, I don't think that this is the use-case of class-based views
(you can achieve the same with function-based views). The whole point
is being able to reuse as much code as possible while allowing to
flexibly modify the view's behavior at a rather fine-grained level.
Take a look at the Feed view in Django (or look at ModelAdmin from the
admin interface if you prefer):
http://docs.djangoproject.com/en/dev/ref/contrib/syndication/
You can customize the query that gets executed and you can customize
every single field that gets displayed in the feed. It's very easy and
flexible and in contrast to function-based views you don't need to
rewrite the whole view from scratch to change a little detail.
Bye,
Waldemar Kornewald
It's still possible in exactly the same way as you do it today:mydata = ...
urlpatterns('', url('...', 'MyView', {'outside_of_lifecycle': mydata}))
Alternatively, you can derive from MyView:
class SubMyView(MyView):
ouside_of_lifecycle = mydata
However, I don't think that this is the use-case of class-based views
(you can achieve the same with function-based views). The whole point
is being able to reuse as much code as possible while allowing to
flexibly modify the view's behavior at a rather fine-grained level.
Take a look at the Feed view in Django (or look at ModelAdmin from the
admin interface if you prefer):
http://docs.djangoproject.com/en/dev/ref/contrib/syndication/
You can customize the query that gets executed and you can customize
every single field that gets displayed in the feed. It's very easy and
flexible and in contrast to function-based views you don't need to
rewrite the whole view from scratch to change a little detail.
You don't seem to get how threading works.
There is no state in this object past the assignments in __init__.
These are thread-safe as there is only one instance of the object.
Variables in local scope are always thread-safe as reetrancy grants
you a new local scope every time it enters a function.
--
Patryk Zawadzki
Attaching all kinds of stuff to self tends to depend on the order of
the methods called and allows for some ugly coding styles. In that
case the slightly strange idea of merging the request and the view by
extending HttpRequest starts to make sense since otherwise you would
have some state on the request object (like the user etc. as it is
now) and some state on the view object.
That said, both extending HttpRequest and using __new__ seem like a
hacks to me. Personally the idea of a view being a callable class
instance or method is simple and flexable enough to handle any use
cases we are talking about. You just have to think of self as the
*view* rather than a thread safe request because that's what it is.
Having a view object per request makes no sense. You should have a
request object per request. It is largely a result of wanting the
convenience of being able to write sloppy code that writes all kinds
of state to self rather than a technical necessity.
Ian
> --
> You received this message because you are subscribed to the Google Groups "Django developers" group.
> To post to this group, send email to django-d...@googlegroups.com.
> To unsubscribe from this group, send email to django-develop...@googlegroups.com.
> For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
>
>
--
=======================================
株式会社ビープラウド イアン・ルイス
〒150-0021
東京都渋谷区恵比寿西2-3-2 NSビル6階
email: ianm...@beproud.jp
TEL:03-6416-9836
FAX:03-6416-9837
http://www.beproud.jp/
=======================================
You, sir, deserve a beer!
--
Patryk Zawadzki
Let's keep the discussion productive and not talk BS.
> There is no state in this object past the assignments in __init__.
> These are thread-safe as there is only one instance of the object.
The one-instance approach is no more thread-safe than having a global
variable. In your example nothing bad will happen, but once we get to
some real-world reusable views you'll quickly see a need for saving
thread-local state somewhere. We had that problem with Django's Feed
class, for example, where we needed access to the request in one of
the methods and the only workaround was to instantiate a new Feed for
every request and manually inject a _request attribute. The point is
that in a class-based view you normally have lots of methods for
customizing the view's behavior, so either you pass the whole
thread-local state as arguments to every single method (which is
*very* ugly and tedious) or you save that state in self. Also, the
problem with passing lots of arguments around is that it can make
adding more variables to the current state and thus customizability
difficult. There's no way around allowing thread-local state and the
one instance per request approach solves this elegantly.
Bye,
Waldemar Kornewald
So the only limitation is that you don't want to pass parameters to methods?
<sarcasm>Good thing self is passed implicitly or classes would be
totally useless!</sarcasm> ;)
More seriously:
Explicit is better than implicit.
Simple is better than complex.
[...]
If the implementation is hard to explain, it's a bad idea.
--
Patryk Zawadzki
Imagine what the code would look like with parameter passing:
class View(object):
def __call__(self, request, **kwargs):
qs = self.get_queryset(request, **kwargs)
.....
def get_queryset(self, request, **kwargs):
...
Now someone writes a reusable view for S3 file management which only
has one parameter (bucket_name) and publishes the code as an
open-source project:
class S3View(View):
def get_queryset(request, bucket_name, **kwargs):
self.get_bucket(bucket_name)
...
...
Then you want to use his code in your project:
class MyS3View(S3View):
def get_bucket(self, bucket_name):
# oh no! I need the request and the other configuration
paramters here! :(
Now you have a problem because the other developer was too lazy to
pass request and **kwargs to that little "unimportant" get_bucket
method. This happens in practice and our job is to come up with a
design that makes this impossible or very unlikely. If the
one-instance-per-request solution solves this and saves you from
typing **kwargs all over the place then it absolutely is better, hands
down.
Bye,
Waldemar Kornewald
>
> That said, both extending HttpRequest and using __new__ seem like a
> hacks to me. Personally the idea of a view being a callable class
> instance or method is simple and flexable enough to handle any use
> cases we are talking about. You just have to think of self as the
> *view* rather than a thread safe request because that's what it is.
I agree about HttpRequest. As Tom pointed out Request is not a View.
It just doesn't fit the inheritance.
As for the __new__, I guess it's just two diffrent approaches and in
the end you get pretty much the same thing. I think of "self" as
"environment" and the class as "view". In one instance approach, the
instance is the "view" and "environment" is in method parameters or
request (is this right?).
Anyway, I think this discussion in on a wrong track. From a Django
user POV, I really don't care how it's implemented. I'm far more
concerned with the API that generic views will provide.
--
Łukasz Rekucki
You have a strong point here. We could at least provide a
new_instance(request, *args, **kwargs) classmethod for unit tests
which automates the __new__ and __init__ calls. Anyway, I'm not sure
which is worth more:
* relatively easy for unit tests (new_instance())
* enforced thread-safety
* no special code in Django's URL routing
vs
* no-brainer for unit tests
* no enforced thread-safety (you can mistakenly create a global view instance)
* special code in Django's URL routing
Bye,
Waldemar Kornewald
Why would you hardcore the queryset inside the view? Pass the
pre-filtered queryset to the constructor and use get_objects_or_404 to
filter it basing on what is passed to __call__.
Actually as I see it __call__ is not the best idea:
class GenericView(object):
def __init__(self, queryset):
self.qs = queryset
def details(self, request, id):
obj = get_object_or_404(self.qs, id=id)
return direct_to_template(request, 'details.html', {'object': obj})
def list(self, request):
objs = get_objects_or_404(self.qs)
return direct_to_template(request, 'list.html', {'objects': objs})
# ... and so on
> Now someone writes a reusable view for S3 file management which only
> has one parameter (bucket_name) and publishes the code as an
> open-source project:
>
> class S3View(View):
> def get_queryset(request, bucket_name, **kwargs):
> self.get_bucket(bucket_name)
> ...
> ...
Are you sure you need to pass the bucket name each time you _call_ the
view? I'd pass it at construction time instead.
> Then you want to use his code in your project:
>
> class MyS3View(S3View):
> def get_bucket(self, bucket_name):
> # oh no! I need the request and the other configuration
> paramters here! :(
Request sure, but what other configuration parameters? Default
implementation should under no circumstances pass and silently swallow
**kwargs.
> Now you have a problem because the other developer was too lazy to
> pass request and **kwargs to that little "unimportant" get_bucket
> method. This happens in practice and our job is to come up with a
> design that makes this impossible or very unlikely. If the
> one-instance-per-request solution solves this and saves you from
> typing **kwargs all over the place then it absolutely is better, hands
> down.
See above.
--
Patryk Zawadzki
> The example he provided isn't terribly good but you don't need an view
> instance per request (the example will work unless you start adding
> stuff to self or overwriting self.qs etc). Only shared state is stored
> at the class level and view customization is done at the class
> instance level as well. That state shouldn't change.
>
> Attaching all kinds of stuff to self tends to depend on the order of
> the methods called and allows for some ugly coding styles. In that
> case the slightly strange idea of merging the request and the view by
> extending HttpRequest starts to make sense since otherwise you would
> have some state on the request object (like the user etc. as it is
> now) and some state on the view object.
>
> That said, both extending HttpRequest and using __new__ seem like a
> hacks to me. Personally the idea of a view being a callable class
> instance or method is simple and flexable enough to handle any use
> cases we are talking about. You just have to think of self as the
> *view* rather than a thread safe request because that's what it is.
>
> Having a view object per request makes no sense. You should have a
> request object per request. It is largely a result of wanting the
> convenience of being able to write sloppy code that writes all kinds
> of state to self rather than a technical necessity.
>
> Ian
I'd like to support Ian and Patryk's position: class based views
should not be instantiated for each request - for all the reasons Ian
gave in the quoted mail.
If you want to avoid passing around (lots of) arguments between
methods, you can always store request specific state on the request
instance, which is a common pattern for middleware already.
To prevent the request's dict from becoming too cluttered, simply put
the arguments into `request.view_context` (a dict or similar
container, needs to be painted).
__
Johannes
So here's an idea, of dubious quality ;) Only pass request around to
methods, and have a requests dictionary on the view (which is
instantiated once) then methods can use that dict to store data in it.
Alex
--
"I disapprove of what you say, but I will defend to the death your
right to say it." -- Voltaire
"The people's good is the highest law." -- Cicero
"Code can always be simpler than you think, but never as simple as you
want" -- Me
Okay, folks: at this point the discussion has pretty much descended
into bikeshedding territory. I'm going to read through everything
posted so far and try to post a summary and round-up to help us get
refocused; gimme a few hours to pull that together and then let's try
to reach towards a consensus.
Jacob
I've started putting everything into a summary spreadsheet. Feel free
to change/extend it:
https://spreadsheets.google.com/ccc?key=0AnLqunL-SCJJdGhxSVZaQkNCcTlzM2d4OEc5dFRPUUE&hl=en
Bye,
Waldemar Kornewald
Some of the approaches also allow for easy grouping of similar views.
For example I'm currently working on an ArchiveView with methods:
* index(self, request)
* details(self, request, year, month, day, id)
* day(self, request, year, month, day)
* week(...)
* month(...)
* year(...)
So you only instantiate one object (telling it which templates to use,
what month format you prefer etc.) and bind its methods to various
URLs.
--
Patryk Zawadzki
Any results?
Bye,
Waldemar Kornewald