I've just added a summary of the last thread on class-based views [1].
This summary isn't 100% complete -- any contributions from interested
parties are welcome. Try to keep opinions to a minimum; this page is
about documenting the strengths and weaknesses of various approaches,
not about expressing opinions. In the same way that CSRF [2] and
session-messages [3] have good wiki pages describing the design
considerations, we need to be able to explain to future generations
why class-based views are the way they are.
Based on the discussion on the most recent thread [4], plus in-person
discussions, I'm proposing that we move forward with Ben Firshman and
David Larlet's "__call__ + copy()" approach. Yes, the copy() is a bit
of a weird idiom, but it's not an idiom that users need to be
particularly concerned with; the code "just works" in an unsurprising
way. The other viable option is the __new__ based copy; however, this
approach is also idiomatically surprising (instantiation returns an
object of type other than the object being constructed), and it
doesn't allow for the use of arguments at time of instantiation.
I'd like to try and get this in for 1.3. It's a big feature, which
means it needs to be in by October 18. However, based on the state of
the patch that Ben has prepared, I think this is an achievable goal.
Three things are required to make this happen:
* Documentation. It looks like David Larlet has made a start on this
already, but there's lots more required.
* Conversion of the github project into a patch on trunk.
* Feedback on the API, especially the generic view replacements.
If you have any feedback, drop it into this thread so that we can
integrate the changes. If you are interested in helping out on the
first two points, drop a line here too so that we can coordinate
activity.
I already have one specific piece of API feedback: the current
implementation requires that all view logic is contained in GET(),
POST() etc style views. This is fine, except for the fact that there
are occasions where GET and POST share a lot of common logic. It would
make sense to me to allow for a request() method that is the direct
analog of how we do views right now, wrapping lines 53-59 of base.py
into a request() method. This would allow users to define their own
dispatch mechanisms, or share logic between GET and POST (and any
other http method) as necessary.
[1] http://code.djangoproject.com/wiki/ClassBasedViews
[2] http://code.djangoproject.com/wiki/CsrfProtection
[3] http://code.djangoproject.com/wiki/SessionMessages
[4] http://groups.google.com/group/django-developers/browse_thread/thread/e23d9a5a58fe5891
Yours,
Russ Magee %-)
I already have one specific piece of API feedback: the current
implementation requires that all view logic is contained in GET(),
POST() etc style views. This is fine, except for the fact that there
are occasions where GET and POST share a lot of common logic. It would
make sense to me to allow for a request() method that is the direct
analog of how we do views right now, wrapping lines 53-59 of base.py
into a request() method. This would allow users to define their own
dispatch mechanisms, or share logic between GET and POST (and any
other http method) as necessary.
This! is! Python! Any shared logic can be in another method, or
function for that matter, and need not be canonicalized in an API. All
that would be needed is an example in the docs:
class TheView(views.ClassView):
....def _common(self, bla):
........# set up models, forms, other preprocessing here
........# return models/forms or set them on class
........# could be run from __init__ or __new__ or whatever we wind up with
....def GET(self, bla):
........self._common(bla)
....def POST(self, bla):
........self._common(bla)
(Indent-dots to make surer that gmail doesn't meddle with the text.)
Surely class-based views shouldn't disallow methods not named nor
given meaning in an API? Since you needed to explicitly wish for a
specific method, that's how it can be interpreted.
HM
Passing things around between '_common' and GET and POST makes a simple
view much more complex than it needs to be, especially when you have
various local variables that you now have to assign in some way. In the
end you will end up just routing it all to the one method:
....def GET(self, *args, **kwargs):
........return self._common(*args, **kwargs)
....def POST(self, *args, **kwargs):
........return self._common(*args, **kwargs)
This is just 4 lines of annoying boilerplate to undo the dispatching
that was 'helpfully' done for you.
I would definitely support a 'request' method (perhaps call it
'dispatch'?) that allows us to avoid that logic. I suspect that every
view I've created that used a form would be hampered by having dispatch
based on HTTP verb done first.
Luke
--
"Despair: It's always darkest just before it goes pitch black."
(despair.com)
Luke Plant || http://lukeplant.me.uk/
> I'd like to try and get this in for 1.3. It's a big feature, which
> means it needs to be in by October 18. However, based on the state of
> the patch that Ben has prepared, I think this is an achievable goal.
I think too but it needs a lot of feedback from the community.
>
> Three things are required to make this happen:
>
> * Documentation. It looks like David Larlet has made a start on this
> already, but there's lots more required.
Yes, only a start but I'm working on it, any help is really appreciated.
> * Conversion of the github project into a patch on trunk.
Do you see that as a total replacement of actual views.generic or should
we go for a old/new dance?
> * Feedback on the API, especially the generic view replacements.
A first feedback from a fork [1], I think it can be interesting to move
the paginate view too.
>
> I already have one specific piece of API feedback: the current
> implementation requires that all view logic is contained in GET(),
> POST() etc style views. This is fine, except for the fact that there
> are occasions where GET and POST share a lot of common logic. It would
> make sense to me to allow for a request() method that is the direct
> analog of how we do views right now, wrapping lines 53-59 of base.py
> into a request() method. This would allow users to define their own
> dispatch mechanisms, or share logic between GET and POST (and any
> other http method) as necessary.
What do you think of [2], is that what you have in mind? I chose
dispatch, as Luke suggested, because request can be confusing.
Best,
David
[1]
http://github.com/josesoa/django-class-based-views/commit/5f24e25e83f63b44b7049a746713bec0cb16e34a
[2]
http://github.com/bfirsh/django-class-based-views/commit/20b63340bbf2b6152f88b5ee47865689d9fae3a6
Few more questions I haven't seen answers yet.
How should urls() work for such views? Should they?
How can self.redirect_to() get current app, namespace and function
name to work if one should use reverse() ?
How should request should be passed for self.render()? Implicitly or explicitly?
You've taken GET/POST dispatch as an example of the new approach, but
I suggest you to use this example (sorry for low-quality draft):
class UserView(View):
templates_prefix = ''
users = User.objects.all()
app_instance = 'users'
list_template = '%slist.html'
edit_template = '%sedit.html'
def template_for(page):
return getattr(self, 'list_' % self.templates_prefix)
@property # as we now have >= python 2.4
def urls(self):
return patterns('',
url(r'^$',
self.list,
name='%s:list'),
url(r'^(.+)/$',
self.edit,
name='%s:edit'),
)
def redirect_to(self, request, page):
#how to get full path to the page from attribute page???
return HttpResponseRedirect(request.build_absolute_uri(request)
+ reverse(self, urlconf=request.urlconf) + reverse(page,
urlconf=self.urls))
def list(self, request):
return self.render(self.template_for('list'), {'users': self.users})
def edit(self, request, id):
if not self.users.filter(id=id).exists():
return self.redirect_to(self.list)
return self.render(self.template_for('edit'), {'user':
self.users.get(id=id)})
class CustomersView(View):
list_template = 'generic_list.html'
edit_template = 'my_edit.html'
users = Users.objects.filter(is_staff=False)
url(r'^detail/users/', CustomersView(), name="users", app_name="users"),
> --
> 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.
>
>
--
Best regards, Yuri V. Baburov, ICQ# 99934676, Skype: yuri.baburov,
MSN: bu...@live.com
There's nothing stopping Django shipping a base view which just calls a
request method, and then another base view which inherits from that and
does method-based dispatch - we shouldn't add too many classes, else
it'll be a documentation nightmare, but that seems a sensible split.
Andrew
I have added a "how to help" section to the wiki page:
http://code.djangoproject.com/wiki/ClassBasedViews
Ben
Could you (or anyone knowledgable) add a section, that explains why each request should have its own view instance?
The thread-safety argument alone does not suffice: if all _request_ state would be stored on request instead of the view, you wouldn't need new instances per request. You could also pass around state explicitly - which admittedly gets messy quickly.
So is this purely about preventing users from shooting themselves in the foot? (hyperbole: Will there be protection from globals in django 1.4?)
> Based on the discussion on the most recent thread [4], plus in-person
> discussions, I'm proposing that we move forward with Ben Firshman and
> David Larlet's "__call__ + copy()" approach. Yes, the copy() is a bit
> of a weird idiom, but it's not an idiom that users need to be
> particularly concerned with; the code "just works" in an unsurprising
> way.
I disagree. If you define a class-based view, create an instance (a singleton) and put it in your urlconf, you wouldn't (or shouldn't) expect it to magically reinstantiate itself everytime it's called. So at least the docs will have to point out the magic prominently.
Regardless of my resentments, thanks for finally creating an official foundation for class based views.
__
Johannes
Without wanting to beat a dead horse, I concur. Fundamentally we need
to have a little faith in your users, the admin has the exact same
pattern of a single instance, and we haven't done anything special
there.
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
Erm... no. A view doesn't have a collection of URLs. A view is a view.
What we're debating here is a way to construct generic views as a
class, rather than as a method with a hideously long argument list.
This isn't a class based view. It's a collection of views, wrapped in
a class. You might be able to call it an app. #6735 isn't about apps -
it's about single views.
Yours,
Russ Magee %-)
Unless you're proposing to maintain backwards compatibility with the
old APIs, we can't just remove the old ones. This essentially means an
old/new arrangement of some kind is required. I'm not especially hung
up on the name, as long as it's simple and memorable.
>> * Feedback on the API, especially the generic view replacements.
>
> A first feedback from a fork [1], I think it can be interesting to move the
> paginate view too.
Sounds like an interesting idea.
>> I already have one specific piece of API feedback: the current
>> implementation requires that all view logic is contained in GET(),
>> POST() etc style views. This is fine, except for the fact that there
>> are occasions where GET and POST share a lot of common logic. It would
>> make sense to me to allow for a request() method that is the direct
>> analog of how we do views right now, wrapping lines 53-59 of base.py
>> into a request() method. This would allow users to define their own
>> dispatch mechanisms, or share logic between GET and POST (and any
>> other http method) as necessary.
>
> What do you think of [2], is that what you have in mind? I chose dispatch,
> as Luke suggested, because request can be confusing.
As I said in my response to Luke -- it's not only confusing; it's
impossible due tot he clash with self.request. Beyond that, it's a bit
of a bikeshed. dispatch(), handle(), or anything else simple and
memorable will be fine.
Yours,
Russ Magee %-)
It's been pointed out to me that we can't really call it request()
anyway, because it would clash with self.request. dispatch() is as
good a name as any; handle() would be my only other suggestion.
However, this really is bikeshedding at this point. Whoever paints it
gets to pick the name.
Yours,
Russ Magee %-)
On Fri, Oct 1, 2010 at 9:53 AM, Vinay Sajip <vinay...@yahoo.co.uk> wrote:
> It seems better to stress thread-safety dos and don'ts in the
> documentation.
Without wanting to beat a dead horse, I concur. Fundamentally we need
to have a little faith in your users, the admin has the exact same
pattern of a single instance, and we haven't done anything special
there.
+1
The thread safety issues certainly aren't insignificant, but I think they can be solved by a big fat warning in the documentation. I do prefer this to the potentially false sense of security that copying the object may involve.
Tobias
--
Tobias McNulty, Managing Partner
Caktus Consulting Group, LLC
http://www.caktusgroup.com
The thread-safety argument is more than enough for me.
*If* all requests state was stored on the request, you're correct -
there wouldn't be a need for a class-based view to be instantiated per
request. However, at that point, you're not really using a class --
you're using a collection of methods that share a sub-module
namespace.
> So is this purely about preventing users from shooting themselves in the foot? (hyperbole: Will there be protection from globals in django 1.4?)
"Don't use global variables" is advice that is contained in any
first-year CS textbook.
"Don't use instance variables on a class" isn't advice you're going to
see printed very often. Yet that would be the advice that would be
*required* in order to use stateless class-based views in the way you
describe.
Objects have state. I don't think it's at all unreasonable to suggest
that a instances of a class based view should be able to have state.
And even if we documented the fact that they can't have state, I'm
willing to *guarantee* that people will *try* to make them stateful.
So - we can:
* spend a bunch of time writing infrastructure to prevent people from
putting stateful variables onto their class-based views
* spend a bunch of time answering the same question over and over
again on django-users
* pick an infrastructure that lets people use classes the way they expect.
>> Based on the discussion on the most recent thread [4], plus in-person
>> discussions, I'm proposing that we move forward with Ben Firshman and
>> David Larlet's "__call__ + copy()" approach. Yes, the copy() is a bit
>> of a weird idiom, but it's not an idiom that users need to be
>> particularly concerned with; the code "just works" in an unsurprising
>> way.
>
> I disagree. If you define a class-based view, create an instance (a singleton) and put it in your urlconf, you wouldn't (or shouldn't) expect it to magically reinstantiate itself everytime it's called. So at least the docs will have to point out the magic prominently.
You say "magic", I say an unusual, but otherwise elegant solution for
hiding an annoying implementation detail.
As an end user, you don't have to care about the fact that it's
"magically" instantiated every time it is used. It just behaves like a
class instance, and is thread safe without you having to think about
it.
Yours,
Russ Magee %-)
Not really. The big win from a class-based view is not being able to
store state, you can do that with local variables, it's being able to
override parts of the behavior without needing to rewrite the entire
view, or add 101 parameters.
>> So is this purely about preventing users from shooting themselves in the foot? (hyperbole: Will there be protection from globals in django 1.4?)
>
> "Don't use global variables" is advice that is contained in any
> first-year CS textbook.
>
> "Don't use instance variables on a class" isn't advice you're going to
> see printed very often. Yet that would be the advice that would be
> *required* in order to use stateless class-based views in the way you
> describe.
>
> Objects have state. I don't think it's at all unreasonable to suggest
> that a instances of a class based view should be able to have state.
> And even if we documented the fact that they can't have state, I'm
> willing to *guarantee* that people will *try* to make them stateful.
>
> So - we can:
> * spend a bunch of time writing infrastructure to prevent people from
> putting stateful variables onto their class-based views
> * spend a bunch of time answering the same question over and over
> again on django-users
> * pick an infrastructure that lets people use classes the way they expect.
>
We don't have that documentation for the admin. Either we need it, or
our users are a little bit smarter than we're giving them credit.
>>> Based on the discussion on the most recent thread [4], plus in-person
>>> discussions, I'm proposing that we move forward with Ben Firshman and
>>> David Larlet's "__call__ + copy()" approach. Yes, the copy() is a bit
>>> of a weird idiom, but it's not an idiom that users need to be
>>> particularly concerned with; the code "just works" in an unsurprising
>>> way.
>>
>> I disagree. If you define a class-based view, create an instance (a singleton) and put it in your urlconf, you wouldn't (or shouldn't) expect it to magically reinstantiate itself everytime it's called. So at least the docs will have to point out the magic prominently.
>
> You say "magic", I say an unusual, but otherwise elegant solution for
> hiding an annoying implementation detail.
>
> As an end user, you don't have to care about the fact that it's
> "magically" instantiated every time it is used. It just behaves like a
> class instance, and is thread safe without you having to think about
> it.
>
> Yours,
> Russ Magee %-)
>
> --
> 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.
>
>
Alex
> Not really. The big win from a class-based view is not being able to
> store state, you can do that with local variables, it's being able to
> override parts of the behavior without needing to rewrite the entire
> view, or add 101 parameters.
If you're storing state with local variables, though, don't you end up having to pass 101 parameters around to your methods?
-Brandon
I'm in agreement that this is the gain from class-based views.
However, as soon as you show someone an instance of a class, they're
going to look for the instance attributes -- that's just how classes
work everywhere else in the world.
Unless you can propose a mechanism for physically preventing people
from assigning instance attributes to the class, I simply don't buy
that documentation saying "don't do that" will be sufficient to avoid
confusion.
>>> So is this purely about preventing users from shooting themselves in the foot? (hyperbole: Will there be protection from globals in django 1.4?)
>>
>> "Don't use global variables" is advice that is contained in any
>> first-year CS textbook.
>>
>> "Don't use instance variables on a class" isn't advice you're going to
>> see printed very often. Yet that would be the advice that would be
>> *required* in order to use stateless class-based views in the way you
>> describe.
>>
>> Objects have state. I don't think it's at all unreasonable to suggest
>> that a instances of a class based view should be able to have state.
>> And even if we documented the fact that they can't have state, I'm
>> willing to *guarantee* that people will *try* to make them stateful.
>>
>> So - we can:
>> * spend a bunch of time writing infrastructure to prevent people from
>> putting stateful variables onto their class-based views
>> * spend a bunch of time answering the same question over and over
>> again on django-users
>> * pick an infrastructure that lets people use classes the way they expect.
>>
>
> We don't have that documentation for the admin. Either we need it, or
> our users are a little bit smarter than we're giving them credit.
Or a third option -- the use case for the admin is different.
ModelAdmin is a pret-a-porter interface to a high level piece of
functionality with a couple of clearly documented extension points.
The API that ModelAdmin exposes doesn't provide any encouragement to
store state on the ModelAdmin instance itself. The fact that
ModelAdmin isn't thread safe in that way isn't documented, because
it's simply never been an issue -- the interface it provides doesn't
encourage you to do things that would cause the thread non-safety to
be exposed.
This contrasts directly with Class-based views: a low-level view
construction kit. Although the interface for the generic class-based
views will be well defined in the same way as the admin, the base View
class isn't. Best practice may well be to avoid state variables, but I
don't think any amount of documentation will stop people from using
them.
Yours,
Russ Magee %-)
This is a very good point - one of the reasons it's nice to have
self.request is so you don't end up with ridiculous method signatures.
It's also a lot easier to write inflexible methods if you don't have
state on self - if I'm in, say, get_form(), and I want to change the
form I have based on the arguments in the URL (or anything elsewhere in
the class), I can't do that unless someone thought to pass those
variables in initially in the method signature.
Andrew
It's not just about stopping users from shooting themselves in the foot,
it's about helping them to do the right thing easily. Without this kind
of protection, it will be harder for *anyone*, including experienced
developers who are aware of the problem, to do things correctly. It's
like the autoescaping in templates - I *know* that I should use the
'escape' filter, but without autoescaping it is hard for anyone to do it
right all the time.
One alternative that has been suggested is to pass state around
explicitly, which is definitely best practice, but in many cases might
not be possible (e.g. if you are working with someone else's base class,
and in one overridden method you want to set up some objects which ought
to be available to other methods). You could also attach stateful data
to the request object, but attaching random bits of data to the request
is surely just as ugly a solution as the self.copy() call, especially if
you need to avoid names clashes with all the other attributes attached
to request.
With regards to doing a shallow copy, it should be noted that at the
point the copy is made, the only data attached to the object is data
that has been attached in the constructor. It is of course possible that
methods within the view might mutate such data, but it seems more likely
that it will be used as immutable configuration data.
However, I have already seen some example code that might fall foul of
this problem - someone gave the example of storing a queryset on the
object in the __init__. This will get implicitly mutated (the cache is
filled) when it is used the first time. This could lead to frustrating
problems that wouldn't be found in testing, and doing copy() on the
instance won't help.
So, in light of the fact that developers *will* need to be aware of this
issue, I'd like to tentatively suggest an explicit solution which is
nonetheless easy to use and get right. We could have an explicit 'state'
object that is thrown away for every new request, something like this:
class State(object):
pass
class View(object):
def __call__(self, request, *args, **kwargs):
"""
Main entry point for a request-response process.
"""
self.state = State()
self.request = request
self.args = args
self.kwargs = kwargs
resp = self.dispatch(request, *args, **kwargs)
# let any state get GC'd immediately:
del self.state
del self.request
del self.args
del self.kwargs
return resp
We document the issue, warn people not to store state on the instance
itself, but tell them that if they must have stateful data, it should be
attached to self.state instead of self, and they will be OK. They might
still be bitten if they put mutable configuration data into the __init__
and it gets mutated, but we have made it easy to do the right thing -
doing 'self.state.x' is only slightly harder than 'self.x'
Thoughts?
We document the issue, warn people not to store state on the instance
itself, but tell them that if they must have stateful data, it should be
attached to self.state instead of self, and they will be OK. They might
still be bitten if they put mutable configuration data into the __init__
and it gets mutated, but we have made it easy to do the right thing -
doing 'self.state.x' is only slightly harder than 'self.x'
Thoughts?
class View(object):
def __call__(self, request, *args, **kwargs):
self.request = request
self.args = args
self.kwargs = kwargs
resp = self.dispatch(request, *args, **kwargs)
self.__dict__ = {} # goodbye state!
return resp
This would kill the ability to use __init__ to store anything on the
instance, since it would only work for the first request, and so would
enforce subclassing as the only means of customising behaviour. It would
be more effective at dealing with thread safety concerns than a
'copy()', but breaking __init__() like that is fairly evil. I'm not sure
if this is a serious suggestion or not...
if you remove the ability to set state in the constructor and rely
only in subclassing use __new__ instead of copy and all the thread
safety problems are solved.
Perhaps we could do something like that and provide a @classmethod in
View that easily subclasses a View with some default state, for
example:
views.py
------------
MyView(View):
...
urls.py
----------
url(r'^myview$', myapp.views.MyView, name='myview'),
url(r'^myview$', myapp.views.MyView.with_defaults(somevar=somevalue),
name='myview'),
If you are willing to limit view state to a dedicated container, you might as well put in on request (e.g. request.view, request.state, request.context, ...) - and store args/kwargs in it as well.
There would be no need to copy or clear self. It could be added automatically for all views (even those that are plain functions).
However, I am -0 on the self.copy() proposal. While I believe it's the wrong design decision, I'd be happy with any decision at this point.
__
Johannes
I may be missing something obvious here, so please tell me if I am..
but couldn't the resolver just check that quacks like something
OOViewish has been passed in and simply branch and init the class
before calling the view in the same way it normally would?
pseudo code follows:
obj = None
func = None
if hasattr(view,something_an_ooview_has): #general ooview instance checking
obj = view()
func = view.dispatch
else:
func = view
response = func(...)
return response
D
url(r'...',MyOOView,...)
Sure. Now decorate the view.
Decorating views in the urlconf is a very common and useful pattern,
but it's a pattern that is pretty much impossible using the approach
you describe.
Yours,
Russ Magee %-)
I guess what I'm getting at is the more general issue of maintaining
the same interface... is it really that important?
If people acknowledge that they are using a different system, then
they can adjust their expectations accordingly.
For example, this specific case of a decorator could be changed
slightly so that the decorator is passed as a parameter to the url
conf, or something like that.
If the goal here is to be completely unsurprising and fit something
into the existing system then I'll let the thread get back on topic :)
I was just considering the possibility of actually providing another
option which is a bit more of a radical change, but also doesn't
involve some quicky ... tweaks..
Sure. Now try and explain what constitutes "mutable state" in a way
that makes sense to new Python converts. I don't see how you can get
into that sort of discussion without getting into deep copying, object
referencing, etc.
> For mutable state during request processing, I'd vote for sticking it
> all in the request. Then your method signatures need only include the
> request - not unreasonable - and if for some reason you couldn't do
> that, there's always thread locals for the requests (duck ;-))
We need to make the distinction here between "ideas that see almost
certainly good ideas/potential best practice" and "things that people
will do because they will expect to be able to do them". I cant deny
that assigning state to the request is one way to avoid these
problems, but it doesn't change the fact that when presented with a
class, people will expect that self.foo will work.
To make matters worse, self.foo *will* work... in the single threaded
devserver. Then you go to production, and everything starts breaking
unpredictably because you have a multithreaded environment.
To make it perfectly clear - any solution that proposes to avoid this
issue purelt with documentation is a non-starter IMHO. I don't care
how well written the description is; not being able to assign to self
is a fundamental change from the expectation of how a class behaves in
every other Python context, and it doesn't take much of a crystal ball
to predict that it will rapidly become a FAQ on django-users. When we
can reasonably predict that something is going to become a support
nightmare, that's the first sign that we need to run the other way.
Now, I'm sure the counterargument is going to be that copy() will be
just as error prone and FAQ inducing. My argument to this is:
* when it fails, it will fail during development, not just in production
* the cases where it will fail isn't the default usage case; it's
only going to be an issue when you have a deep data structure that you
are using in a mutable way.
* the cases where it is even *potentially* a problem are an edge
case of common usage. This is only an issue with the arguments passed
to the constuctor of the class-based view. One of the reasons for
moving to class based views is to get away from the monster list of
arguments required by method based generic views in favor of
subclassing. By virtue of the technique we're introducing, we're
limiting the number of people that will be affected by this issue.
I'm a lot more comfortable with the idea of documenting "don't use
complex data structures as class arguments" than I am about
documenting "don't use self on a class". If nothing else, the
reasoning of "a copy is involved, so make __init__ arguments cheap to
copy" will give some protection.
Yours,
Russ Magee %-)
Agreed. There is already an object to store state per request. It's
the request object. It's there for a reason. Adding data to the
class instance would be like adding data to the view function (like
view_function.data = "spam"). No one would agree that's a good idea.
> We don't have that documentation for the admin. Either we need it, or
> our users are a little bit smarter than we're giving them credit.
Playing devil's advocate, the admin may not be accessed as much as the
main site, so bugs involving concurrent access may not crop up as much
or be prioritized lower depending on the organization creating the
website.
--
Ian
By this reasoning, when you add extra helper methods to your class,
which is the reason for OO views, you need to add/use the data those
methods will modify to the request object? That just doesn't make
sense to me at all. It reminds me of the "Feature Envy" code smell..
D
I'm not sure I understand your comment. Are you complaining about having to pass
the request object around? I think having a helper view add or modify
data on the request
object is more than reasonable. Please also explain to me what you are
meaning by "Feature Envy".
class ViewClass(...):
...
def helper_method(self, request):
request.somedata = "data"
...
I think there might be a nicer way to do this like adding a state
dictionary to the request so that
there was a place to put request state that's not directly on the
request, but that feels like
bikeshedding at this point, if I used the term correctly.
As Alex has said earlier, this is the way the Admin has always worked
and it works ok IMHO so it makes
sense to do something similar with non-admin class based views.
--
Ian
I can't argue with the fact that setting variables in __init__() is a
common idiom in Python generally, and this is certainly a weakness of
copy on call that will fail in non-thread safe ways.
My counterclaim is that the move to class-based views is largely
driven by a desire to discourage the use of arguments in this way. We
want to discourage people from using arguments to class-based views,
and use subclassing instead.
Given that we're discouraging people from using arguments to
__init__(), and that it *will* work for arguments with "simple" data
types, I consider it *marginally* more acceptable than giving users a
class-like structure that straight out won't work like any other class
they've ever used.
Lets be clear here - I don't particularly like *any* of the options on
the table so far. However, my opinion is that copy on call is the
option that will provide the least surprising behavior for the most
common use cases -- having a subset of arguments to __init__ being
unsafe is less surprising to me that having *all* attributes on self
either unsafe, or actively prohibited.
>> I'm a lot more comfortable with the idea of documenting "don't use
>> complex data structures as class arguments" than I am about
>> documenting "don't use self on a class". If nothing else, the
>> reasoning of "a copy is involved, so make __init__ arguments cheap to
>> copy" will give some protection.
>
>> Sure. Now try and explain what constitutes "mutable state" in a way
>> that makes sense to new Python converts. I don't see how you can get
>> into that sort of discussion without getting into deep copying, object
>> referencing, etc.
>
> I don't understand how these two paragraphs are internally consistent.
> I agree that explaining the intricacies of mutable vs immutable
> objects to beginners is impractical. So how then is it reasonable to
> document "you can set instance attributes in __init__ as long as they
> are immutable," which is what would have to be documented in the copy-
> on-call case?
Agreed, there isn't much distinction. I suppose it's a matter of
degrees. A simple "avoid lists and dicts as __init__ args" is an
easier paragraph than something that tries to complicate matters by
talking about readonly state and writable state, and mutable data an
immutable data. The core of the matter isn't actually that different -
it's just that you don't need all the extra framing complicating
matters.
> ISTM that of the thus-far proposed options for "preventing" un-thread-
> safe use of class-based views without changing the view contract, the
> only one that will fail fast in development is the proposal to make
> setting state on self raise an error or warning.
Another option would be to use copy-on-call, but raise an error if
they provide any arguments to __init__(). This would be annoying and
counter to Python idiom, but it strikes me as less fundamentally
counterintuitive than prohibiting *any* use of self to store state.
We could even wrap the "no args to __init__" error check in a method
that enables it to be overridden and silenced in a subclass; that way,
introducing the potentially un-threadsafe behavior would need to be an
active action on the part of the developer. Then we can document the
"feature" up the wahoo -- "If you *really* need arguments to your
class-based view's constructor, you can get them, but here's the
caveats... if you ignore these caveats, here's what will happen"
Yours,
Russ Magee %-)
Now you lost me. If we are discouraging people from using arguments to
__init__, why does the page list "You can't use arguments to init() to
instantiate a class view" as a drawback of the __new__ based approach
?
Anyway, using mutable arguments to view's __init__ is pretty much the
same as using mutable objects as default values for function
arguments. Doing:
def myview(request, options, defaults={'foo': 'bar'}):
defaults.update(options)
.....
will fail even with only one thread (this is a quite common mistake of
novice python programmers). So will using a dictionary as an argument
to __init__. The difference is, that it's easy to explain the function
case without going into much details. In case of __init__ arguments I
can't seem to find a good explanation not involving phrase "global
variable" which I prefer not to use.
My proposal it to drop __init__ args (which actually reduces the
copy/call to the new/call approach). If you still want to set some
options in your urls.py and 2 lines to make a subclass is too much of
boilerplate, here's a friendly helper (you can make it a method of
view):
def with_args(view_cls, **kwargs):
return type(view_cls.__name__, (view_cls,), kwargs)
# in urls.py:
(r'^somepath', with_args(MyView, option=False)),
# or even
(r'^somepath', MyView.with(option=False)),
--
Łukasz Rekucki
I second that view. We should tell people how to write thread-safe
code, not try to work around broken solutions. Thread-safe views are
easy. Just don't write to self once you're out of __init__.
Workarounds are not trivial, can make your life hard in wonderful ways
(like "why is __new__ called twice?") and intentionally break best
practices. I'd really prefer to teach people not to stab themselves
instead of forcing everyone to wear a shiny set of armor.
--
Patryk Zawadzki
The difference is that __new__ doesn't *ever* allow for initialization
arguments -- there is no way to pass an argument in. An "arguments
disabled by default" __init__+copy implementation allows arguments as
an opt-in.
There's also the wierd behavior that __new__ requires. Consider:
x = MyView()
What is x? If you use the __init__+copy approach, x is an instance of
MyView. If you use __new__, x is a HttpResponse. It's a minor thing,
to be sure, but this is a bit of a code smell to me.
> Anyway, using mutable arguments to view's __init__ is pretty much the
> same as using mutable objects as default values for function
> arguments. Doing:
Yes - but that's something that is well known as a Python gotcha. The
same restriction doesn't usually apply to just passing values to a
constructor.
> def myview(request, options, defaults={'foo': 'bar'}):
> defaults.update(options)
> .....
>
> will fail even with only one thread (this is a quite common mistake of
> novice python programmers). So will using a dictionary as an argument
> to __init__. The difference is, that it's easy to explain the function
> case without going into much details. In case of __init__ arguments I
> can't seem to find a good explanation not involving phrase "global
> variable" which I prefer not to use.
>
> My proposal it to drop __init__ args (which actually reduces the
> copy/call to the new/call approach). If you still want to set some
> options in your urls.py and 2 lines to make a subclass is too much of
> boilerplate, here's a friendly helper (you can make it a method of
> view):
>
> def with_args(view_cls, **kwargs):
> return type(view_cls.__name__, (view_cls,), kwargs)
>
> # in urls.py:
>
> (r'^somepath', with_args(MyView, option=False)),
> # or even
> (r'^somepath', MyView.with(option=False)),
I can't deny this would work. It just doesn't feel very Pythonic to me.
Yours,
Russ Magee %-)
Opinion noted, but as I've said before, I fundamentally disagree.
The argument here isn't whether it's easy to write a thread-safe view.
It's whether you can accidentally write a thread unsafe view by
following the practices you have used consistently in every other
piece of code you have written.
Python classes have state. People *will* assign variables to self,
because that's something they have done with every other Python class
in existence. And as soon as their code hits production, their code
will break, in unpredictable ways. Claiming that users should
RTFM/should know better isn't an acceptable excuse in this case, IMHO.
When an API will break when you use it in the obvious way, that isn't
something you can hide in documentation and say "we told you so".
Yours,
Russ Magee %-)
Classes that represent real objects have state. Like cars have color,
make and registration number, your models have attributes that
differentiate them from other objects of same class.
"Utility" classes (usually singletons) that represent a function
(service) should not have state.
Imagine walking into a bank and talking to a clerk. She asks for your
name and purpose. You say you want to withrdraw some money. She then
walks up to a giant whiteboard, wipes it and writes your name and your
bank account number there. Then she yells "Joooeeeey" and walks away.
But until Joey appears to give you money, you see another clerk walk
up to the whiteboard, wipe your data and replace it with John Doe. She
then proceeds to exclaim "Joooeeeey" and walks away. That's what a
stateful view is :)
--
Patryk Zawadzki
s/until/before/
--
Patryk Zawadzki
Ok, now show me keyword in Python that identifies a class as a
"utility class". Or the page in the Python documentation where it
talks about "utility classes" and how you identify them and use them
properly.
You can't, because Python doesn't make that distinction. Python has
classes. Class instances have state. Period. People certainly write
classes that are stateless. But providing a base API based around
classes, encouraging people to subclass and add their own behavior,
and expecting *documentation* to prevent them from using self is
complete folly IMHO.
An API that depends on an unenforced contract that you *don't* perform
an action that is otherwise completely normal and accepted behavior
is, in my opinion, prima facie broken. It rates somewhere between
either a 3 and a -4 on Rusty Russell's API design scale [1][2],
depending on how generous you're being. By my reckoning, the
"init-disabled call+copy" rates somewhere in the 5-7 range.
[1] http://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html
[2] http://ozlabs.org/~rusty/index.cgi/tech/2008-04-01.html
> Imagine walking into a bank and talking to a clerk. She asks for your
> name and purpose. You say you want to withrdraw some money. She then
> walks up to a giant whiteboard, wipes it and writes your name and your
> bank account number there. Then she yells "Joooeeeey" and walks away.
> But until Joey appears to give you money, you see another clerk walk
> up to the whiteboard, wipe your data and replace it with John Doe. She
> then proceeds to exclaim "Joooeeeey" and walks away. That's what a
> stateful view is :)
Analogies only work when they:
1) make sense
2) are actually analogous.
Neither of these appear to be the case here. The best way I can "fix"
this analogy is to point out that every time you're walking into the
bank, a completely new bank with a clean whiteboard is constructed for
you, and you're the only patron who is allowed to use the bank before
it is demolished.
Yours,
Russ Magee %-)
Please ignore this suggestion, and my previous one, if you didn't
already; I don't know what I was thinking, because they don't address
the multi-threaded problem at all.
Luke
--
"DO NOT DISTURB. I'm disturbed enough already."
Luke Plant || http://lukeplant.me.uk/
> Having said that, I'm in favour of the first approach mentioned in the
> wiki (store state on request) and I'm surprised it doesn't seem to
> have any traction in this thread. The only argument against is "it's
> unusual to have a class where you can't store state on self" but even
> that can be overcome with an appropriate __setattr__/__getattr__ that
> access self.request.state under the hood. Unless someone tries
> self.__dict__[attr], it will look as if the state is stored on the
> view although it's actually stored in the request (or better
> request.state to avoid name clashes). Perhaps I am missing something
> but this seems to me foolproof and much cleaner than the alternatives.
This breaks if you set any values in __init__ - since the request object
doesn't exist to store state on.
This largely depends on what you mean by "initialization". There are
actually two levels of this: 1) per-request initialization based on
the request 2) per-view initialization on application startup.
The __init__ method in current implementation[1] falls in to the
second category (independently of the ability to pass in any arguments
as noted by Carl Meyer). This kind of settings should never mutate for
the reasons given by Vince Veselosky. Adding a rule: "don't mutate
objects assigned to self in __init__" doesn't really feel like a
normal use of a Python class.
I agree with you that, subclassing is the prefered way of customizing
views. The fact that class attributes are shared by instances should
be pretty clear to any Python programmer with OOP knowledge (same as
the fact that default arguments for functions are shared between
invocations).
To sum this up, I think the important questions are:
1) Do View instances need to share anything ? I say: yes.
2) What is the purpose of this shared objects ?
I say: customization of *default* view settings (like default
redirect path, default template, default page size in pagination).
3) How do we make this shared settings immutable between requests
(this has nothing to do with thread-safety) ?
This is largely the topic of this thread. Deep copying it
everytime is surely the most secure option, but not very efficient.
Another way is to keep this settings as class attributes and assume
the user is sane enough not to share mutable object between class
instances.
>
> There's also the wierd behavior that __new__ requires. Consider:
>
> x = MyView()
>
> What is x? If you use the __init__+copy approach, x is an instance of
> MyView. If you use __new__, x is a HttpResponse. It's a minor thing,
> to be sure, but this is a bit of a code smell to me.
I don't consider this weird. The view's contract is: "give me a
request, I'll give you a response". In OOP terms, it's a just a
Factory of HttpResponses.
>
>> Anyway, using mutable arguments to view's __init__ is pretty much the
>> same as using mutable objects as default values for function
>> arguments. Doing:
>
> Yes - but that's something that is well known as a Python gotcha. The
> same restriction doesn't usually apply to just passing values to a
> constructor.
Same common knowledge exists concerning class attributes. Lets reuse that.
>>
>> def with_args(view_cls, **kwargs):
>> return type(view_cls.__name__, (view_cls,), kwargs)
>>
>> # in urls.py:
>>
>> (r'^somepath', with_args(MyView, option=False)),
>> # or even
>> (r'^somepath', MyView.with(option=False)),
>
> I can't deny this would work. It just doesn't feel very Pythonic to me.
I don't share your feeling about this. Django already uses a similar
idiom in ModelForms - you pass in a base form, some extra options and
get a new subclass. Also, I would consider this as a shortcut for
advanced users and advice beginners to use subclassing.
[1]: http://github.com/bfirsh/django-class-based-views
--
Łukasz Rekucki
There is no Python documentation that teaches you how to write good
and robust code. There are only docs that tell you what "print" or
"raise" does. I wouldn't pick software engineering during my studies
if I expectes language references to teach me all there was about
programming.
> You can't, because Python doesn't make that distinction. Python has
> classes. Class instances have state. Period. People certainly write
> classes that are stateless. But providing a base API based around
> classes, encouraging people to subclass and add their own behavior,
> and expecting *documentation* to prevent them from using self is
> complete folly IMHO.
I was going to respond to that. I wanted to mention things like
compiled languages and their expectance of you being able to write
good code. I respect you too much to drag you through that. After all
I am not forced to use whatever the standard becomes. If I am ever
forced to use this sort of class-based views, I'll file bugs, until
then I'll try to remain quiet. There are more important decisions to
be made.
Cheers,
--
Patryk Zawadzki
Agreed. The "just document" approach is a non-starter; the 'error when
you set attribute on self' is at least a technically viable option.
My problem with the error-based approach is entirely conceptual -- it
feels to me like throwing the baby out with the bathwater. I'm having
a lot of trouble with the idea that class instances have state in
every other Python class instance, and if we introduce an API that
physically prohibits instance state that -- no matter how good the
technical reasoning -- we're going to:
* Cause confusion amongst new users as to why this seemingly obvious
thing can't be done
* Ignore the legitimate occasions where using state is a useful
architectural approach.
>> We could even wrap the "no args to __init__" error check in a method
>> that enables it to be overridden and silenced in a subclass; that way,
>> introducing the potentially un-threadsafe behavior would need to be an
>
> Again, arguments to __init__ are not the issue. What would have to be
> checked is any assignment to self that takes place in __init__. How do
> you propose to check that?
I don't. You've got me -- in my haste to propose an idea, I hadn't
considered assigning state in the constructor.
I'm thinking on my feet at the moment, but the only way I can think to
resolve this would be to remove the copy() call; effectively making
the __call__() a factory that produces a new class instance that
shares all the methods of the class-based view, but none of the state
created at initialization. However, this is starting to get into the
same territory as why the __new__ approach isn't particularly
appealing.
Yours,
Russ Magee %-)
In fairness, my position of "consensus" was based on the many previous
mailing list discussions that have occurred, as well as the outcome of
a couple of in-person discussions during sprints of the major players.
It's clear from this revival of the discussion that this consensus
wasn't as strong as I thought it was.
I completely agree that we don't want to rush this. The upside is that
if we *can* reach consensus, it isn't going to require a whole lot of
code changes; We're arguing about how to architect the base class, but
once we've got that sorted out, Ben's patch is feature complete and
shouldn't be too hard to adapt to any base class along the lines of
what we've been describing.
> Having said that, I'm in favour of the first approach mentioned in the
> wiki (store state on request) and I'm surprised it doesn't seem to
> have any traction in this thread. The only argument against is "it's
> unusual to have a class where you can't store state on self" but even
> that can be overcome with an appropriate __setattr__/__getattr__ that
> access self.request.state under the hood. Unless someone tries
> self.__dict__[attr], it will look as if the state is stored on the
> view although it's actually stored in the request (or better
> request.state to avoid name clashes). Perhaps I am missing something
> but this seems to me foolproof and much cleaner than the alternatives.
I can see two problems here:
* You still need to assign instance variables during construction; if
you call self.foo during __init__(), you don't have a request; so this
means we need to dynamically swap in a request-based
__setattr__/__getattr__ as part of __call__.
* The more complicated things are, the more likely they are to break
in interesting and creative ways. Futzing with __getattr__/__setattr__
(especially if it's happening dynamically during class use) strikes
me as the sort of thing that has the potential to backfire in really
unusual ways, especially if people get comfortable with the idea that
this class-based view really is just a class, and they start pushing
the envelope.
Yours,
Russ Magee %-)
Agreed.
> 2) What is the purpose of this shared objects ?
> I say: customization of *default* view settings (like default
> redirect path, default template, default page size in pagination).
Shared settings from the constructor -- agreed. Instance variables
that form part of view logic are another (albiet related) issue.
> 3) How do we make this shared settings immutable between requests
> (this has nothing to do with thread-safety) ?
It's entirely about thread safety; if you've got code modifying any
instance variable, and those instance variables don't have isolated
state between class instances, thread safety will be the first
manifestation.
> This is largely the topic of this thread. Deep copying it
> everytime is surely the most secure option, but not very efficient.
There are also some objects that aren't deep-copyable.
> Another way is to keep this settings as class attributes and assume
> the user is sane enough not to share mutable object between class
> instances.
... and that's a *really* big assumption, and one that has potential
to bite users on the butt really easily -- especially new developers.
>>> def with_args(view_cls, **kwargs):
>>> return type(view_cls.__name__, (view_cls,), kwargs)
>>>
>>> # in urls.py:
>>>
>>> (r'^somepath', with_args(MyView, option=False)),
>>> # or even
>>> (r'^somepath', MyView.with(option=False)),
>>
>> I can't deny this would work. It just doesn't feel very Pythonic to me.
>
> I don't share your feeling about this. Django already uses a similar
> idiom in ModelForms - you pass in a base form, some extra options and
> get a new subclass. Also, I would consider this as a shortcut for
> advanced users and advice beginners to use subclassing.
I'm not sure I see the comparison. ModelForms are declared using a
metaclass; instances are defined in the normal way that every class is
defined. There is a modelform_factory() et al, but they're not class
methods; they're standalone functions that can be used to construct
new classes.
MyView.with(xxx) -- a class method that is a class factory -- isn't a
construct with precedent in Django's codebase AFAICT.
Yours
Russ Magee %-)
I _really_ like the idea of View being synonymous with a ResponseFactory.
Using __call__:
The view base class can take *args and **kwargs and apply them lazily
when __call__ is called. This is basically like the copy+call one,
except there is no copy,
class ResponseFactory: #might as well be called View
def __init__(self,*args,**kwargs): # discourage args/kwargs
self.args = args
self.kwargs = kwargs
def __call__(self,*args,**kwargs):
response = self.__class__(*self.args,
**self.kwargs).dispatch(*args, **kwargs)
return response
No copy. An explicit contract, with an understood pattern (Factory)
that the initial args and kwargs will be applied to every instance
created from this one (and thus should be considered volatile).
Even though its trivially different, it feels a lot cleaner in my head :)
D
> --
> 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.
>
>
While I'm in the "one singleton view instance is best" camp and think
that storing some state on the request and some on the view is a bit
gross, I understand Russell's arguments. New users are simply going to
save stuff on self no matter how much we complain, document etc. It's
simply a reality that likely can't be helped much.
Other frameworks seem have View/Handler instances per request, such as
appengine's webapp so there is some precedent for creating an instance
per request.
http://code.google.com/appengine/docs/python/gettingstarted/handlingforms.html
Flask seems to do it the callable singleton way:
http://flask.pocoo.org/docs/patterns/lazyloading/
Is there any precedent in other frameworks for doing it the singleton
way? Talking a bit about how other frameworks/libraries do this might
be a good idea to figure out what Joe User is likely to do.
--
Ian
What I meant is that you don't need multi-threaded environment to make
the problem occur - just 2 sequential requests.
>
>> Another way is to keep this settings as class attributes and assume
>> the user is sane enough not to share mutable object between class
>> instances.
>
> ... and that's a *really* big assumption, and one that has potential
> to bite users on the butt really easily -- especially new developers.
IMHO, it falls into the same class of assumptions as "people won't use
mutable objects as default arguments to functions". It's true that
it's a common mistake among developers new to OOP in Python. But it's
a Python-specific problem, not Django-specific.
>>>> def with_args(view_cls, **kwargs):
>>>> return type(view_cls.__name__, (view_cls,), kwargs)
>>>>
>>>> # in urls.py:
>>>>
>>>> (r'^somepath', with_args(MyView, option=False)),
>>>> # or even
>>>> (r'^somepath', MyView.with(option=False)),
>>>
>>> I can't deny this would work. It just doesn't feel very Pythonic to me.
>>
>> I don't share your feeling about this. Django already uses a similar
>> idiom in ModelForms - you pass in a base form, some extra options and
>> get a new subclass. Also, I would consider this as a shortcut for
>> advanced users and advice beginners to use subclassing.
>
> I'm not sure I see the comparison. ModelForms are declared using a
> metaclass; instances are defined in the normal way that every class is
> defined. There is a modelform_factory() et al, but they're not class
> methods; they're standalone functions that can be used to construct
> new classes.
>
> MyView.with(xxx) -- a class method that is a class factory -- isn't a
> construct with precedent in Django's codebase AFAICT.
We can leave this as a standalone function if that changes anything.
Another common pattern in use is an instance method that is a instance
factory. Going up one level seems natural.
PS. I'll try to put up some code today, to represent this approach. Is
it okay to add this to the wiki page ?
--
Łukasz Rekucki
> Is there any precedent in other frameworks for doing it the singleton
> way? Talking a bit about how other frameworks/libraries do this might
> be a good idea to figure out what Joe User is likely to do.
+1 on having a compare table between other frameworks.
--
Łukasz Rekucki
>>>>> def with_args(view_cls, **kwargs):
>>>>> return type(view_cls.__name__, (view_cls,), kwargs)
>>>>>
>>>>> # in urls.py:
>>>>>
>>>>> (r'^somepath', with_args(MyView, option=False)),
>>>>> # or even
>>>>> (r'^somepath', MyView.with(option=False)),
>>>>
>>>> I can't deny this would work. It just doesn't feel very Pythonic to me.
>>>
>>> I don't share your feeling about this. Django already uses a similar
>>> idiom in ModelForms - you pass in a base form, some extra options and
>>> get a new subclass. Also, I would consider this as a shortcut for
>>> advanced users and advice beginners to use subclassing.
>>
>> I'm not sure I see the comparison. ModelForms are declared using a
>> metaclass; instances are defined in the normal way that every class is
>> defined. There is a modelform_factory() et al, but they're not class
>> methods; they're standalone functions that can be used to construct
>> new classes.
>>
>> MyView.with(xxx) -- a class method that is a class factory -- isn't a
>> construct with precedent in Django's codebase AFAICT.
>
> We can leave this as a standalone function if that changes anything.
> Another common pattern in use is an instance method that is a instance
> factory. Going up one level seems natural.
>
> PS. I'll try to put up some code today, to represent this approach. Is
> it okay to add this to the wiki page ?
Feel free. Even if we don't end up adopting your proposal, it's
helpful for future generations to know that it *was* proposed (and if
it wasn't accepted, why it wasn't accepted).
Yours,
Russ Magee %-)
I don't think you'll find any argument that having an instance per
request would solve all the problems that this thread has described.
The issue is how to declare a view that is able to be instantiated on
a instance-per-request basis while simultaneously allowing decorators
to be easily wrapped around that view.
Yours,
Russ Magee %-)
I'm not a flask expert either but under "Loading Late" there is a
LazyView which I believe is just a callable object.
And later under that he adds a LazyView instance to a url rule so I think it's
simply a "view is a callable" pattern.
--
Ian
I was sort of referring to the __new__() method you are describing to
allow state to be stored on self.
A class instance that can be wrapped by a decorator to me is simply a
callable instance and people are just
going to have to get used to the idea that self can't be modified
safely. Anything else is a bad hack that will likely
cause even more headaches down the road.
That said, even Django itself has code that falls victim to storing
state on self in a callable class based view (See:
django.contrib.formtools.preview.FormPreview). So I can imagine many
people will be confused, but it is what it is.
--
Ian
I'd just like to add more noise to the signal and reiterate this -
storing state on self (or request) leads to much cleaner, less fragile,
more subclassable views (in my own experience, having written both a set
of generic views with and without state like this).
I personally think self is the place to store this state (since, as Russ
keeps saying, that's how nearly all Python classes do it), but storing
it on request isn't too bad - it just "feels" more wrong to me, but
then, middleware does it already.
However, subjectivity isn't going to win through here. Either solution
is fine, just don't force me to pass everything around as arguments
between functions. That's not looking likely, but some people here seen
to want to have that kind of purity at the expense of usability -
dammit, Jim, I'm an engineer, not a mathematician.
Andrew
I'd just like to add more noise to the signal and reiterate this - storing state on self (or request) leads to much cleaner, less fragile, more subclassable views (in my own experience, having written both a set of generic views with and without state like this).
I personally think self is the place to store this state (since, as Russ keeps saying, that's how nearly all Python classes do it), but storing it on request isn't too bad - it just "feels" more wrong to me, but then, middleware does it already.
Seriously though, the key considerations and points made in this thread need to be extracted and put somewhere in the django docs, preferably in a place that doesn't distract a brand new user but also gets read soon after they complete the tutorial and begin digging a little deeper. Knowing the design considerations that went into whatever path is taken is, IMHO, invaluable.
I am willing to do the work to make that happen if nobody else is interested. I have not yet contributed to the corpus of django documentation, but perhaps this an opportunity to do so.
However, subjectivity isn't going to win through here. Either solution is fine, just don't force me to pass everything around as arguments between functions. That's not looking likely, but some people here seen to want to have that kind of purity at the expense of usability - dammit, Jim, I'm an engineer, not a mathematician.
This is problematic if I want to return something that isn't a context,
or like it (for example, if I'm rendering images, or fetching content
wholesale out of a database).
So, bfirsh's previous iteration had content formats built into the base
class - I ripped it out and replaced it with a simpler base class, and
he seemed to agree, so that's where we currently are.
My main concern is getting rid of the simplicity - e.g. of calling
render() on a template/context mix. In this aforementioned previous
iteration, if I wanted to supply custom context to a custom template, I
had to override both self.get_template_name() and self.get_context() -
even though it would have been a lot easier to override GET and just
call self.render(). It's a similar problem to passing everything around
as keyword arguments - reusability doesn't turn out to be much better
than starting from scratch.
(FWIW, I've written several hundred LOC with both styles and it was a
lot less work when there was less abstraction)
I just don't want us to build in all these abstraction layers and have
the ability to customise everything perfectly but in a verbose fashion.
That said, if render_result() in your example just calls a normal
render() method itself, it's easy to not use it if you don't have to, so
a reasonable approach to get both of these angles in is possible.
Also, what use cases do people have for returning arbitary pages as JSON
or XML?
I'm not saying there aren't any - far from it - but I much prefer
evidence to speculation, so some concrete examples would be great
Finally, we have to be wary of making this automatic on all views, or
something similar - it could potentially be a
security/warm-fuzzy-feeling risk, as some people put things into the
context that aren't necessarily for display to the user (though you'd
have to be pretty stupid to put anything actually sensitive in there).
(There's also the issue of whether you apply context processors to the
JSON replies, and so forth, but that's going even further down the
rabbit hole)
Andrew
Given that the primary complain against the __new__ solution is that
it's unintuitive to Python programmers that instantiating their class
results in some other class, why not simply go with a more explicit
classmethod. Simply used as:
url("^articles/$", ArticlesView.dispatch).
It seems to address all concerns: it's explicit, safe to assign
attributes, Pythonic, and fails loudly if used improperly (using
AritlcesView would result in an Exception being throw since it doesn't
return an HttpResponse, and using, and ArticlesView() an error about
not being callable).
Last idea, I swear,
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
Interesting. So `View.dispatch` would look something like:
@classmethod
def dispatch(cls, request, *args, **kwargs):
instance = cls()
callback = getattr(instance, request.method.lower())
return callback(request, *args, **kwargs)
(e.g. create an instance of `cls` then call `get`/`post`/etc. on it).
Other than the slight cruftyness of `dispatch`, I quite like it. Seems
it addresses most of the issues we've found so far. Doesn't solve the
"pass configuration into __init__" idea, but frankly I didn't think
much of that idea in the first place.
To my eyes this is the best solution raised so far.
Jacob
> Given that the primary complain against the __new__ solution is that
> it's unintuitive to Python programmers that instantiating their class
> results in some other class, why not simply go with a more explicit
> classmethod. Simply used as:
>
> url("^articles/$", ArticlesView.dispatch).
>
> It seems to address all concerns: it's explicit, safe to assign
> attributes, Pythonic, and fails loudly if used improperly (using
> AritlcesView would result in an Exception being throw since it doesn't
> return an HttpResponse, and using, and ArticlesView() an error about
> not being callable).
>
> Last idea, I swear,
> Alex
>
I've put together a repos with a bunch of scripts that demonstrate in
about a page of code the essence of what we are trying to achieve
(without all the detail about the actual dispatch to HTTP verbs etc).
Start with naive.py:
http://bitbucket.org/spookylukey/django-class-views-experiments/src/tip/naive.py
(Just run as 'python naive.py' to see the output). Browse up a level to
see the other variants.
I implemented Alex's latest idea in classmethod.py (with 'as_view'
instead of 'dispatch', to avoid name clashes). It suffers from the same
limitation as the __new__ in that you can't pass configuration data to
the init. But you can do a 'configure' class method, as you can also do
with __new__
Personally, I now prefer these: first place:
http://bitbucket.org/spookylukey/django-class-views-experiments/src/tip/classmethod.py
Close second:
http://bitbucket.org/spookylukey/django-class-views-experiments/src/tip/magicnew_with_configure.py
Given the limitations, I'm no longer that bothered about the fact that
the __new__ method returns something completely unexpected, but Alex's
classmethod solution seems like a nice improvement on it, though it
makes a URLconf very slightly more verbose.
Luke
--
"Doubt: In the battle between you and the world, bet on the world."
(despair.com)
Luke Plant || http://lukeplant.me.uk/
The only problem is decorators: You can't just simply apply
login_required() to the class or the dispatch() method. Otherwise, I
like this approach.
--
Łukasz Rekucki
> The only problem is decorators: You can't just simply apply
> login_required() to the class or the dispatch() method. Otherwise, I
> like this approach.
It works fine:
http://bitbucket.org/spookylukey/django-class-views-experiments/src/tip/classmethod.py#cl-40
(I have 'as_view' instead of 'dispatch'). My MyView.dispatch method
could have decorators applied inside the actual class (as long as they
are adjusted for being method decorators and not function decorators, in
common with all other solutions).
After playing around with this for a bit, it seems to be the best option
on the table. The only slight API problem I've come across is that it
is *possible* to try to pass configuration arguments to __init__:
class MyView(View):
def __init__(self, myparam=0):
...
view = MyView(myparam=1).as_view
instead of using configure:
view = MyView.configure(myparam=1).as_view
However, as soon as you try to use that view, you will find that your
custom parameter is being ignored. That might cause some head
scratching to the newbie, but nothing like the insidious thread-safety
problems we're trying to avoid.
You're right, my bad. It's 8pm and i'm still in work - probably
shouldn't read any more code today :). That said, my preferences would
be: magicnew_with_configure, classmethod, magicnew, copyoncall.
--
Łukasz Rekucki
At the risk of being silly again, this can be prevented by a decorator:
class classonlymethod(classmethod):
def __get__(self, instance, owner):
if instance is not None:
raise AttributeError("This method is availble only on the
view class.")
return super(classonlymethod, self).__get__(instance, owner)
Of course, subclasses that want to override as_view() would need to
use the same decorator (classmethod will still work, just won't catch
the error).
>
> Luke
>
>
> --
> "Doubt: In the battle between you and the world, bet on the world."
> (despair.com)
>
> 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-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.
>
>
--
Łukasz Rekucki
> Last idea, I swear,
I didn't swear, so here is another slight variation :-) You actually
*call* the classmethod in your URLconf, passing any constructor
arguments to it:
url(r'^detail/author/(?P<pk>\d+)/$',
views.AuthorDetail.as_view(some_param=123),
name="author_detail"),
It returns a newly created view function, which in turn calls your
class.
http://bitbucket.org/spookylukey/django-class-views-experiments/src/tip/classmethod2.py
This was, in fact, suggested by Jari Pennanen in May :-(
http://groups.google.com/group/django-developers/msg/997ac5d38a96a27b
It's slightly less verbose for passing parameters, as you don't need a
separate 'configure' call.
Russ mentioned this one can't be decorated.
I personally love the classmethod idea, so that's +1 from me for that.
D
Yes, but that's the sort of asymmetry I'd like to avoid if possible.
It's easy to document that you can wrap a decorator around anything
you can put in a URL pattern. When you start making exceptions, then
you have to explain the exceptions, and then explain to the people who
don't read and/or understand the explanation... and so on.
Yours,
Russ Magee %-)
This looks to me like it could be a winner. The fact that
AuthorDetail(foo=bar).as_view(pork=spam) ignores the 'foo' argument is
a minor wart, but a lot less concerning to me that the potential
threading problems in copy+call, or the 'no state on self' options.
Thanks for working up the examples, Luke!
Russ %-)
Ok - so to kick the process into the next phase, I've just created a
Django branch in my bitbucket repo [1] to cover introducing this to
trunk.
I've used the implementation in Ben's github repo [2] as a starting
point -- it looks like David Larlet has already made the changes to
implement the classmethod2 approach.
So far, other than some reorganization to get everything into Django's
tree, I've made two changes:
* Added PendingDeprecationWarnings to the old function-based generic views
* Following a late night IRC conversation, I've done some PEP8
bikeshedding, and renamed the request methods GET(), POST() etc to
lower case get(), post().
Some other issues that still need to be resolved:
* Does RequestFactory need to be added to Django's test tools
(Possibly fixing #9002)?
* Does django.views.generic.utils.coerce_put_post() indicate a change
that needs to be made in Django? (Is there an existing ticket for
this?)
* Are there any outstanding tickets on generic views that will be
closed by merging this branch, and do they ask for any features that
aren't fixed by this branch?
* We need to reference and topic docs to describe the new view classes.
* We need to update the tutorial to use the new view classes
Pull requests gratefully accepted for any of these items :-)
[1] http://bitbucket.org/freakboy3742/django
[2] http://github.com/bfirsh/django-class-based-views
Yours
Russ Magee %-)
I gave this a quick review and nothing huge jumped out. Looks good so far.
One point of concern that came up though: looking at the way as_view
introduces a closure, it occurs to me that the docstring of am
as_view'd class view isn't very useful, which'll break introspection
and things like the admindocs tool. And just using functools.wraps (or
the equivalent) is as_view won't exactly work, either: you'll get the
dispatch() docstring instead.
So unless anyone can think of a reason why it'd be a bad idea, I think
as_view needs to monkeypatch the class's docstring into the returned
closure. Bit of a code smell, but I think it maintains the correct
self-documenting nature of a view, yeah?
> * Does RequestFactory need to be added to Django's test tools
> (Possibly fixing #9002)?
I'm not sure it's directly class-based-views-related, but I believe it
should, yes. It's a useful pattern that makes it into a utils module
in nearly every app I write.
> * Does django.views.generic.utils.coerce_put_post() indicate a change
> that needs to be made in Django? (Is there an existing ticket for
> this?)
Yeah, this has been a wart in Django for a while -- Django doesn't
really "get" PUT very well. Along the same lines,
request.raw_post_data is terribly named. I don't know that there's a
single ticket anywhere, but I'd like to see a general cleanup here.
I don't see this as a blocker for class-based views, though: we have a
narrow feature deadline that I'd like to hit, and then a longer
bug-fix period we can use to clean up PUT/POST and such.
> * Are there any outstanding tickets on generic views that will be
> closed by merging this branch, and do they ask for any features that
> aren't fixed by this branch?
Almost certainly yes :)
We could *really* use a volunteer who can wade through the open
tickets, look at all the GV-related tickets, and answer this question
concretely.
Anyone? Bueller?
Jacob
That's what I've done on quite a few projects and its worked great. As Russell pointed out I lose decoratoring in urls.py, but that's something I don't use anyway.
--
Andy McKay
an...@clearwind.ca
twitter: @andymckay
> One point of concern that came up though: looking at the way as_view
> introduces a closure, it occurs to me that the docstring of am
> as_view'd class view isn't very useful, which'll break introspection
> and things like the admindocs tool. And just using functools.wraps (or
> the equivalent) is as_view won't exactly work, either: you'll get the
> dispatch() docstring instead.
>
> So unless anyone can think of a reason why it'd be a bad idea, I think
> as_view needs to monkeypatch the class's docstring into the returned
> closure. Bit of a code smell, but I think it maintains the correct
> self-documenting nature of a view, yeah?
Sounds fine to me, and I think it should also copy other function
attributes from the dispatch method, to allow people to apply decorators
like 'csrf_exempt' to the dispatch method and have it work as expected.
We might like to think about whether we add any other mechanism for
decorators. We already have the ability to decorate the result of
'as_view()', which is best suited for things like 'cache_page', and to
decorate methods like dispatch, which is best suited for decorators that
are required for the view to work correctly. A list of decorators could
also be specified as a class/instance attribute and added inside the
'as_view' to the closure that is returned. For the moment, I think
adding this would add too many choices, and if there is a need for it we
can add it as a feature later.
Jacob Kaplan-Moss a �crit :
#1282: archive_today generic view should accept a month_format parameter
instead of hardcoding '%b'
Done, through MonthView.month_format
#2367: pagination for date based generic views
Depends on http://github.com/bfirsh/django-class-based-views/pull/2
#8378: Generic views cache-like behavior
Last edited one year ago, not sure if it's still relevant
#13753: Generic views don't redirect to an URL name, like
django.shortcuts.redirect does
Done, no more post_save_redirect argument but View.redirect_to()
#13842: XViewMiddleware fails with class-based views
Must be fixed before class-based views merge.
I hope I didn't forget one, it's really hard to search with Trac...
Best,
David
(few more found in this query[1])
#9150: Generic views should accept arguements for form_class (related to #12392)
Mostly fixed by adding "initial" parameter in FormMixin. This could be
changes to get_initial(), so it can be based on request object.
#12669: Return different HttpResponse from direct_to_template
Use a TemplateView with get_response() overriden. The patch on the
ticket adds a response class as a parameter. Should we add a similar
to TemplateView ?
#13953: Generic CRUD views: support for callable in post_ACTION_redirect
There are no post_ACTION_redirect parameters anymore, can be done by
overriding redirect_to()
--
Łukasz Rekucki
> Ok - so to kick the process into the next phase, I've just created a
> Django branch in my bitbucket repo [1] to cover introducing this to
> trunk.
Russell - beware - I think bitbucket has managed to create a very broken
clone. I did a fork as well, and both of our repositories are missing
thousands of commits. I can do a pull from django/django and I then get
a complete repository which then seems to be compatible with
django/django. I've e-mailed the bitbucket folks, but have left my
clone as it is for now to allow them to work out what has gone wrong.