On Saturday 11 February 2006 14:47, Russell Keith-Magee wrote: > > I think the main issue is whether we want methods > > that can alter data on the QuerySet directly > > I don't follow - why is direct modification of the query set a bad > thing? Is the problem one of expected behaviour, or is it an internal > operation problem?
I wasn't saying it was a bad thing, just highlighting the separation that exists at the moment between the 'Manager' class and the 'QuerySet' class. But I was wrong anyway - the current implementation does have delete() on QuerySets, just not add(), remove() and clear(). It wouldn't be possible to have add() on QuerySet (at least not if .filter() was used), but remove() and clear() would be possible I think. > > This would change whether you > > would have to do things like reporter.article_set.all(), or just > > reporter.article_set. > > I thought the implied .all() thing was knocked on the head a few > weeks back because of problems with caching: The example I gave was a related object lookup, and so the caching problem doesn't apply. The Manager objects persist since they are attached to the model class, but the RelatedManager objects aren't. > > Also, without your patch, you could do > > reporter.article_set.delete() (whether this was desired or not), > > with it you would have to do reporter.article_set.all().delete(). > > Personally I'm for removing the need to do .all() with the > > RelatedManagers, in both cases. > > Intentional, because of safety. If you want to bulk delete, you have > to explicity nominate that you want to delete all() the elements of > the set. This is especially important in avoiding accidental > confusion between reporter.article_set.clear() and > reporter.article_set.delete() (which isn't outside the realm of > possibility with new users, or old users who haven't had enough sleep > :-). For the delete() case, I guess I'm not that bothered either way about having to do .all(), since I don't need to do bulk delete that often. However, whether for the Manager or the RelatedManager, it's a bit of a deviation from the other methods - for all the others, the Manager has proxy methods to the QuerySet. It might be confusing to miss this one out. Having a special case in a core API simply to make something *harder* to do seems a bit ugly, and fairly un-pythonic too - IMO the 'alters_data' attribute protects non-Python developers and that is enough. I don't actually find the whole safety argument very convincing. I keep a back up of my database, and if I were to accidentally issue a command like Articles.objects.delete(), I would have absolutely no-one to blame but myself - what did I expect it to do? Even without reading the API docs, I could have guessed, and if I didn't bother to read the docs for an API that is used to manage my database I'm doubly to blame. The only time I'm likely to execute .delete() in a I-wonder-what-this-does kind of way is when I'm messing around learning the API, which I'm not really going to do with a real, un-backed-up database. With .delete() on a set of related objects, I can see there is slightly more room for confusion, but then again it can't get much simpler than "delete() deletes things from the database." I'm actually more bothered by the .all() required everytime you use related lookups to actually get the objects (which isn't what this thread is really about, I know). There is no reason for it technically (the reason for Managers was caching and persistence), and it kind of breaks the name - it is no longer an 'article_set', it is something that gives you an article set if you do 'all()' on it. Since 'article_set' could easily be accessed in templates (I have lots of instances in my own project), I think using it needs to be simpler. Luke -- Life is complex. It has both real and imaginary components. Luke Plant || L.Plant.98 (at) cantab.net || http://lukeplant.me.uk/
