On 2/11/06, Luke Plant <[EMAIL PROTECTED]> wrote: > > On Saturday 11 February 2006 08:51, Russell Keith-Magee wrote: > > > Comments? Have I got it right this time? > > 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? Side Note - while I was thinking about this comment, I found a problem with my patch. The QuerySet.delete() method needs to have 'self._result_cache = None' added to the end to ensure that the cache is cleared after the delete is performed. This means that if the query set that was deleted is reused, it will be re-evaluated (as an empty set). I've made this change locally, and modified the last many-to-many unit test to test for it. > 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: http://groups.google.com/group/django-developers/browse_thread/thread/f64127c9b63e2ae5/3362cb72dc6641cf Is this change still on the table? > 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 :-). > This can be easily fixed > by setting the 'alters_data' attribute on the delete() function. > Nitpicking: these line in the tests need to be changed: I've added these two fixes, and updated the patch on the ticket with these two changes, plus the cache clearing modification. Russ Magee %-)
