On Jun 26, 5:12 am, Malcolm Tredinnick <[EMAIL PROTECTED]> wrote: > One thing that jumps out from a quick read: there are no tests on the > patch. > > One test that springs to mind is to create two instances of the same > database row, change different fields and then save each one, re-query > the database and prove the two saves only updated their respective > columns. There might be some other cases that need testing two > (relations, for example?).
I added a test along the lines of your suggestion, is it about what you had in mind? :) > My second thought is that overriding __setattr__ like that has some > drawbacks. It means that if any model in an application implements > __setattr__, they have to remember to call the Model version as well or > else nothing in core -- or any third-party app that was intended to work > in conjunction with other, unknown apps -- could ever assume the > parallel update condition holds in the future, if we wanted to use it. > It also slows things down on the fast path a bit (when this feature > isn't needed). The original version had the list of columns to save > being passed into Model.save(). That feels less intrusive. I'm not particularly tied to either method, so long as some form of this makes it in I'll be happy :) I also found (and fixed) an issue regarding __init__ differences between a model coming from the db and one you make yourself, but it requires an extra kwarg on __init__ - this may not be optimal so that may make things lean back towards the former method of adding an arg to save, but I'm not sure. -- Collin Grady --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [EMAIL PROTECTED] For more options, visit this group at http://groups.google.com/group/django-developers?hl=en -~----------~----~----~----~------~----~------~--~---
