On 1/26/06, Adrian Holovaty <[EMAIL PROTECTED]> wrote: > > On 1/25/06, Russell Keith-Magee <[EMAIL PROTECTED]> wrote: > > Comments on this thread seem to have gone dead - are there any > > objections out there to me committing this patch (the newer patch with > > object delete deferring to the manager)? > > Yeah -- I'm not 100% comfortable with coupling object instances to a > manager. It seems a bit wonky...
I'll grant that the mechanism of using the 'default' manager for the delete is not entirely wonk-free. However: 1) the duplication of the delete code grates against me much more than using a default manager to stimulate SQL production, 2) having the manager defer to the object doesn't really seem right to me (although, I could work the patch this way, as well) 3) Hugo made some interesting points about keeping all SQL code in the manager. This is a first cut at that goal. The next step would be to get save(), and other SQL producing calls into the Manager, along with object remembering which manager was used to produce them. The recent discussion regarding multiple managers suggested that one of the potential use cases for managers was to have multiple backends for objects; if this is the case, SQL code will have to move to the manager, anyway. > Also, the patch appears to have removed support for the DELETE_ALL > safety mechanism (unintentionally?). No - its still there. I had a closer look at the logic when I was working on the manager-based patch. The first thing manager.delete() does is count the number of arguments, then pop off the DELETE_ALL. The check for obliteration is based on the number of arguments as originally counted. If you have 0 arguments initially, you are (accididentally) deleting all - if you add the DELETE_ALL, you argument count will be 1, even though the DELETE_ALL argument is never really used. Therefore, there is no need to explicitly test the DELETE_ALL condition. There is a unit test in 'basic' that validates the revised logic. Russ Magee %-)
