Re: #3182 -- model instance update() method and QuerySet update_or_create() method
On Mon, Mar 23, 2009 at 12:44 AM, Ivan Sagalaev wrote: > I'd like to chime in with another point of view. Not my own, I was just > in a discussion about it in our local forums and I don't want the idea > to vanish. > > A person there wanted an `update()` method too but not behaving like you > show. Instead he wanted it to update *just* the fields passed as > arguments. I.e. it should be equivalent to this: So more like http://code.djangoproject.com/ticket/4102 then ? :) -- 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 django-developers@googlegroups.com To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/django-developers?hl=en -~--~~~~--~~--~--~---
Re: #3182 -- model instance update() method and QuerySet update_or_create() method
Are the objections being raised for `update()` equally applicable to `update_or_create()`? I would have thought that a name-space clash for this method on the objects manager would have been a non-issue, as would any confusion about the use of `force_update` and `force_create` internally (as they are implied in the method name, they should be used internally for each respective option). I agree about the `default` name mis-match, but I can easily live with that as it provides the same interface as `get_or_create()`. Cheers. Tai. On Mar 23, 10:54 pm, Russell Keith-Magee wrote: > On Mon, Mar 23, 2009 at 7:57 AM, Malcolm Tredinnick > > wrote: > > > Kind of disappointed that none of the other "commit at will" people have > > chimed in on this one (Adrian? Jacob? Russell? Bueller?...) I suspect > > I'm going to lose, but I'd genuinely like to know that there's something > > more than apathy behind the approval to add this. > > I'm -0, for mostly the same reasons as Malcolm. > > * "fairly common" is hard to quantify, and the code that is being > replaced isn't that hard to hand-roll as an end-user. > * If we include it, there's a backwards incompatibility issue to deal with. > * My goat (or is that pony?) entrails agree with Malcolm's > predictions of "what about insert()/when to use force_update" > discussions. > > My opinion on the 'code cleanliness' issue is acutally the opposite to > Jacob - I actually prefer the obj.x=val; obj.save(force_update=True) > version, for "explicit over implicit" reasons. However, as Jacob > notes, this is a beauty in the eye of the beholder thing. > > I also have a slight problem with update_or_create method as > described. For me, 'default' implies that any existing value will take > precedence, but in update_or_create, the 'defaults' are effectively > the new values. This is a bit of a bikeshed thing, but it does grate. > > However, I don't have any particularly compelling reason to _not_ > include this feature. My -0 is purely my perception of the balance of > benefits vs risks, but it's a pretty fine balance. I won't lose much > sleep over this either way. > > Russ %-) --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/django-developers?hl=en -~--~~~~--~~--~--~---
Re: #3182 -- model instance update() method and QuerySet update_or_create() method
Malcolm Tredinnick wrote: > On Mon, 2009-03-23 at 10:44 +0300, Ivan Sagalaev wrote: >> Looks like a "+0" useful API wrapper around direct calling of >> _default_manager. > > Would they also like a stable and saddle with that pony? Actually, this very concept has been somewhat explained to the guy [1] :-). [1]: http://softwaremaniacs.org/forum/django/10090/#45895 --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/django-developers?hl=en -~--~~~~--~~--~--~---
Re: #3182 -- model instance update() method and QuerySet update_or_create() method
On Mon, Mar 23, 2009 at 7:57 AM, Malcolm Tredinnick wrote: > > Kind of disappointed that none of the other "commit at will" people have > chimed in on this one (Adrian? Jacob? Russell? Bueller?...) I suspect > I'm going to lose, but I'd genuinely like to know that there's something > more than apathy behind the approval to add this. I'm -0, for mostly the same reasons as Malcolm. * "fairly common" is hard to quantify, and the code that is being replaced isn't that hard to hand-roll as an end-user. * If we include it, there's a backwards incompatibility issue to deal with. * My goat (or is that pony?) entrails agree with Malcolm's predictions of "what about insert()/when to use force_update" discussions. My opinion on the 'code cleanliness' issue is acutally the opposite to Jacob - I actually prefer the obj.x=val; obj.save(force_update=True) version, for "explicit over implicit" reasons. However, as Jacob notes, this is a beauty in the eye of the beholder thing. I also have a slight problem with update_or_create method as described. For me, 'default' implies that any existing value will take precedence, but in update_or_create, the 'defaults' are effectively the new values. This is a bit of a bikeshed thing, but it does grate. However, I don't have any particularly compelling reason to _not_ include this feature. My -0 is purely my perception of the balance of benefits vs risks, but it's a pretty fine balance. I won't lose much sleep over this either way. Russ %-) --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/django-developers?hl=en -~--~~~~--~~--~--~---
Re: #3182 -- model instance update() method and QuerySet update_or_create() method
On Mon, 2009-03-23 at 10:44 +0300, Ivan Sagalaev wrote: [...] > A person there wanted an `update()` method too but not behaving like you > show. Instead he wanted it to update *just* the fields passed as > arguments. I.e. it should be equivalent to this: > > def update(self, **kwargs): > self._default_manager.filter(pk=self.pk).update(**kwargs) > > Looks like a "+0" useful API wrapper around direct calling of > _default_manager. Would they also like a stable and saddle with that pony? It's the same net effect as the method Gary is proposing (providing the object already exists in the database). Even more identical when we add the optional ability to only update the fields that have changed. It's already only a one-liner, as you notice, if somebody wants to do it right now. Regards, Malcolm --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/django-developers?hl=en -~--~~~~--~~--~--~---
Re: #3182 -- model instance update() method and QuerySet update_or_create() method
Jacob Kaplan-Moss wrote: > I've mostly stayed out because it's not something I feel strongly > about, but I am +0 on the change. The reason I don't much care is that > it really comes down to a lines of code argument; is:: > > obj.update(x=1, y=2, z=3) > > really that much "better" than:: > > obj.x = 1 > obj.y = 2 > obj.z = 3 > obj.save() I'd like to chime in with another point of view. Not my own, I was just in a discussion about it in our local forums and I don't want the idea to vanish. A person there wanted an `update()` method too but not behaving like you show. Instead he wanted it to update *just* the fields passed as arguments. I.e. it should be equivalent to this: def update(self, **kwargs): self._default_manager.filter(pk=self.pk).update(**kwargs) Looks like a "+0" useful API wrapper around direct calling of _default_manager. --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/django-developers?hl=en -~--~~~~--~~--~--~---
Re: #3182 -- model instance update() method and QuerySet update_or_create() method
On Sun, Mar 22, 2009 at 5:57 PM, Malcolm Tredinnick wrote: > Kind of disappointed that none of the other "commit at will" people have > chimed in on this one (Adrian? Jacob? Russell? Bueller?...) I suspect > I'm going to lose, but I'd genuinely like to know that there's something > more than apathy behind the approval to add this. I've mostly stayed out because it's not something I feel strongly about, but I am +0 on the change. The reason I don't much care is that it really comes down to a lines of code argument; is:: obj.update(x=1, y=2, z=3) really that much "better" than:: obj.x = 1 obj.y = 2 obj.z = 3 obj.save() Beauty is in the eye of the beholder. I *do* prefer the previous, but really not by a whole lot. Really, though, I can't see much of a reason *not* to add it. The only ``update`` methods I've got that this would clash with is an ``update(**kwargs)`` I've written myself; this would replace it. A quick poke through Google's code search doesn't find any (public) Django code that seems to have an update method. The same argument here, either way, goes for update_or_create, btw. Let's not "compromise" and just take one or the other; that'd just be silly. One or the other, please. Jacob --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/django-developers?hl=en -~--~~~~--~~--~--~---
Re: #3182 -- model instance update() method and QuerySet update_or_create() method
On Mon, Mar 23, 2009 at 7:57 AM, Malcolm Tredinnick wrote: > > Kind of disappointed that none of the other "commit at will" people have > chimed in on this one (Adrian? Jacob? Russell? Bueller?...) I suspect > I'm going to lose, but I'd genuinely like to know that there's something > more than apathy behind the approval to add this. Apologies - I've been AFK for the last three days, and was pretty busy at work leading up to that. I should have some time this evening to look at this. Russ %-) --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/django-developers?hl=en -~--~~~~--~~--~--~---
Re: #3182 -- model instance update() method and QuerySet update_or_create() method
On Sun, 2009-03-15 at 12:44 -0700, Dan Watson wrote: > On Mar 15, 1:12 pm, "Gary Wilson Jr." wrote: > > What do you think? > > Wouldn't this be a backwards-incompatible change at this point? It > would clash with any model fields named "update". No, because of a technicality. We considered this when writing the API stability document, as Alex pointed out. However, I also consider that (Dan's logic) an argument not add something like this unless it's really, really adding otherwise missing functionality. Just because we can, doesn't mean we should exploit the possibility to add to the namespace. That was also one of my reasons, if people search back through the archives, for why save() takes force_update and force_insert parameters, instead of adding two extra methods. If people want to add update() methods to their own models (perhaps via a new base class they inherit from), they can do that already, as they're in control of their own names. But removing a common name from the list of available ones if a two sided coin (or an eight sided die or many splendoured thing, not sure of the right metaphore here). Kind of disappointed that none of the other "commit at will" people have chimed in on this one (Adrian? Jacob? Russell? Bueller?...) I suspect I'm going to lose, but I'd genuinely like to know that there's something more than apathy behind the approval to add this. Regards, Malcolm --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/django-developers?hl=en -~--~~~~--~~--~--~---
Re: #3182 -- model instance update() method and QuerySet update_or_create() method
> I'm very unconvinced that the idea in the ticket is worth it. It's a > two-liner if somebody wants to do this in their code, so whilst we *can* > add this method, adding yet another thing to Model namespace that also > adds to the documentation and things that need to be tested, etc, > doesn't seem worth it to me. We already have save(force_update=True) to > do an update. Adding the extra line to also populate some model fields > doesn't seem like a great API addition that we can't live without to me. I'm probably just missing the obvious here, but what is the two-liner alternative? I thought that the alternative was to put all of the values you want to update in a dict and iterate through that dict and calling `setattr()` and then calling `save()` (like the `update()` method does) or just assign each value to your instance one line at a time and then call `save()`? I can live with that if cluttering the namespace is really that bad, but it does seem fairly inconvenient. Perhaps an `update_fields` argument could be added to `save()` (in keeping with `force_update` and `force_create`), and just move the code from `update()` into `save ()` if that argument isn't empty? I would really like to see `update_or_create()` implemented, though. Personally, I've found a need to use this pattern many times, and having this method to compliment `get_or_create()` would be far more convenient than the example Gary Wilson has given. There shouldn't be too much concern about this method clashing with anything in the Manager namespace. Similar to the above, I suppose a `force_update` argument could be added to `get_or_create()`, but that seems contradictory to the methods meaning as described by its name. I'm +0 for `Model.update()` and +1 for `Manager.update_or_create()`. Cheers. Tai. --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/django-developers?hl=en -~--~~~~--~~--~--~---
Re: #3182 -- model instance update() method and QuerySet update_or_create() method
On Sun, 2009-03-15 at 23:11 -0500, Gary Wilson Jr. wrote: > On Sun, Mar 15, 2009 at 4:40 PM, Malcolm Tredinnick > wrote: > > On Sun, 2009-03-15 at 12:12 -0500, Gary Wilson Jr. wrote: > >> Another option would be to allow Model.update() to take the > >> force_update and force_insert parameters that save() does and just > >> pass these through to the save() call. > > > > Aside from the fact that I dislike the ideal as a whole (see below), if > > you're going to have a method called update(), it should do what it says > > on the box. It's not called maybe_create_maybe_update() -- we already > > have that method (it's spelled "save()"). > > This is the way I lean too, having update() use > save(force_update=True). We already have Model.__init__ and > Manager.create() for creation. > > >> What do you think? > > > > I'm very unconvinced that the idea in the ticket is worth it. It's a > > two-liner if somebody wants to do this in their code, so whilst we *can* > > add this method, adding yet another thing to Model namespace that also > > adds to the documentation and things that need to be tested, etc, > > doesn't seem worth it to me. We already have save(force_update=True) to > > do an update. Adding the extra line to also populate some model fields > > doesn't seem like a great API addition that we can't live without to me. > > Sure, the model update() method is a convenience that we can live > without, but I would argue that it rounds out CRUD for models and > makes things a bit more readable: > > obj.update(field1=value1, field2=value2) So let me predict the obvious future here: (1) "Can we also have an insert() method?" times about 5 times over the next 12 months. (2) "When do I use update? What is the difference between update and save(force_update)?" times any number of times on django-users. (3) "The documentation is unclear / has formatting errors / whatever" (more work required). I think it adds multiple choices when one already exists. I use force_update=True a fair bit and I don't see this actually making code easier, since that type of pattern doesn't seem to come up that much. My models are populated a bit more incrementally (and the loop just isn't hard). For people who do use this pattern a lot, I'm already on record on this list encouraging them to add update() and insert() to their model classes if they want. > With Model.update() and Manager.update_or_create(), we are saving two > to five lines of code for operations that are fairly common. "Fairly common" is hard to quantify. We're guessing pretty blindly here. I'm assuming you use the pattern a bit; as I've noted, I do not. I'm at most -0 on both options. Malcolm --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/django-developers?hl=en -~--~~~~--~~--~--~---
Re: #3182 -- model instance update() method and QuerySet update_or_create() method
On Sun, Mar 15, 2009 at 4:40 PM, Malcolm Tredinnick
wrote:
> On Sun, 2009-03-15 at 12:12 -0500, Gary Wilson Jr. wrote:
>> Another option would be to allow Model.update() to take the
>> force_update and force_insert parameters that save() does and just
>> pass these through to the save() call.
>
> Aside from the fact that I dislike the ideal as a whole (see below), if
> you're going to have a method called update(), it should do what it says
> on the box. It's not called maybe_create_maybe_update() -- we already
> have that method (it's spelled "save()").
This is the way I lean too, having update() use
save(force_update=True). We already have Model.__init__ and
Manager.create() for creation.
>> What do you think?
>
> I'm very unconvinced that the idea in the ticket is worth it. It's a
> two-liner if somebody wants to do this in their code, so whilst we *can*
> add this method, adding yet another thing to Model namespace that also
> adds to the documentation and things that need to be tested, etc,
> doesn't seem worth it to me. We already have save(force_update=True) to
> do an update. Adding the extra line to also populate some model fields
> doesn't seem like a great API addition that we can't live without to me.
Sure, the model update() method is a convenience that we can live
without, but I would argue that it rounds out CRUD for models and
makes things a bit more readable:
obj.update(field1=value1, field2=value2)
instead of:
obj.field1 = value1
obj.field2 = value2
obj.save(force_update=True)
IMO, the update_or_create is a bigger win, since otherwise you would
have something like:
defaults = {'field2': value2, 'field3': value3}
obj, created = get_or_create(filed1=value1, defaults=defaults)
if not created:
obj.update(**defaults)
...which, by the way, without a Model.update() method would look like:
defaults = {'field2': value2, 'field3': value3}
obj, created = get_or_create(filed1=value1, defaults=defaults)
if not created:
for k, v in defaults.iteritems():
setattr(obj, k, v)
obj.save(force_update=True)
...but with update_or_create looks like:
obj, created = update_or_create(filed1=value1, defaults={'field2':
value2, 'field3': value3})
With Model.update() and Manager.update_or_create(), we are saving two
to five lines of code for operations that are fairly common.
Gary
--~--~-~--~~~---~--~~
You received this message because you are subscribed to the Google Groups
"Django developers" group.
To post to this group, send email to django-developers@googlegroups.com
To unsubscribe from this group, send email to
django-developers+unsubscr...@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/django-developers?hl=en
-~--~~~~--~~--~--~---
Re: #3182 -- model instance update() method and QuerySet update_or_create() method
On Sun, 2009-03-15 at 12:12 -0500, Gary Wilson Jr. wrote: [...] > Another option would be to allow Model.update() to take the > force_update and force_insert parameters that save() does and just > pass these through to the save() call. Aside from the fact that I dislike the ideal as a whole (see below), if you're going to have a method called update(), it should do what it says on the box. It's not called maybe_create_maybe_update() -- we already have that method (it's spelled "save()"). > What do you think? I'm very unconvinced that the idea in the ticket is worth it. It's a two-liner if somebody wants to do this in their code, so whilst we *can* add this method, adding yet another thing to Model namespace that also adds to the documentation and things that need to be tested, etc, doesn't seem worth it to me. We already have save(force_update=True) to do an update. Adding the extra line to also populate some model fields doesn't seem like a great API addition that we can't live without to me. Malcolm --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/django-developers?hl=en -~--~~~~--~~--~--~---
Re: #3182 -- model instance update() method and QuerySet update_or_create() method
On Mar 15, 4:40 pm, Jacob Kaplan-Moss wrote: > We take a slightly more reasonable approach to backwards compatibly as > detailed > athttp://docs.djangoproject.com/en/dev/misc/api-stability/#what-stable- > Read that carefully: nothing there prevents us from adding a method > named update. Gotcha. I'll have to pursue http://code.djangoproject.com/ticket/5741 again :-) Dan --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/django-developers?hl=en -~--~~~~--~~--~--~---
Re: #3182 -- model instance update() method and QuerySet update_or_create() method
On Sun, Mar 15, 2009 at 2:44 PM, Dan Watson wrote: > Wouldn't this be a backwards-incompatible change at this point? It > would clash with any model fields named "update". Technically, yes. However, if we interpret the backwards-compatibility requirement this strictly it basically prevents us adding any new features since someone, somewhere, might have defined something with that name, too. We take a slightly more reasonable approach to backwards compatibly as detailed at http://docs.djangoproject.com/en/dev/misc/api-stability/#what-stable-means. Read that carefully: nothing there prevents us from adding a method named update. What'll actually happen, by the way, if someone's got a field named `update` is that the field will actually shadow the method, not the other way around. That's good because it doesn't outright break user code, but it's still generally a booboo. We can -- and should -- add a validation warning that complains about a field named `update`. Jacob --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/django-developers?hl=en -~--~~~~--~~--~--~---
Re: #3182 -- model instance update() method and QuerySet update_or_create() method
On Sun, Mar 15, 2009 at 2:44 PM, Dan Watson wrote: > > On Mar 15, 1:12 pm, "Gary Wilson Jr." wrote: > > What do you think? > > Wouldn't this be a backwards-incompatible change at this point? It > would clash with any model fields named "update". > > Dan > > > Take a look at the backwards compatibility policy, it outlines that we don't consider the APIs complete, and that we can add stuff in the future: http://docs.djangoproject.com/en/dev/misc/api-stability/ 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 --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/django-developers?hl=en -~--~~~~--~~--~--~---
Re: #3182 -- model instance update() method and QuerySet update_or_create() method
On Mar 15, 1:12 pm, "Gary Wilson Jr." wrote: > What do you think? Wouldn't this be a backwards-incompatible change at this point? It would clash with any model fields named "update". Dan --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/django-developers?hl=en -~--~~~~--~~--~--~---
