Opened 8 years ago

Closed 5 years ago

Last modified 2 years ago

#5741 closed Uncategorized (wontfix)

make queryset get(), latest(), earliest(), take a default kwarg

Reported by: dcwatson Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords: queryset get latest earliest default
Cc: hv@…, bignose Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Just to follow up on http://groups.google.com/group/django-developers/browse_frm/thread/1f03320d4640dee/e2647a413161b4a0?hl=en so it doesn't get forgotten. I know this is supposed to wait for the queryset refactoring to be completed, but I figured it couldn't hurt to have a working patch and doc changes now. And as it stands now, the patch would look virtually identical against the queryset-refactor branch.

Attachments (3)

get_default.diff (1.9 KB) - added by dcwatson 8 years ago.
5741.diff (2.3 KB) - added by dcwatson 7 years ago.
updated patch against r8194
get_with_default.diff (2.6 KB) - added by mtredinnick 7 years ago.
Updated patch (just the docs bit changed)

Download all attachments as: .zip

Change History (14)

Changed 8 years ago by dcwatson

comment:1 follow-up: Changed 8 years ago by Thomas Güttler <hv@…>

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Yes, a default argument (like a dictionary) would be good.

Since 95% I use the primary key if I use get(), I suggest this
dict like API:

foo=Foo.objects.get(myid, default)

this should behave like

try:
    foo=Foo.objects.get(**{Foo._meta.pk.name: myid})
except Foo.DoesNotExist:
    foo=default

comment:2 Changed 8 years ago by Thomas Güttler <hv@…>

  • Cc hv@… added

comment:3 in reply to: ↑ 1 Changed 8 years ago by Will Hardy

Or maybe something like:

  foo = Foo.objects.default(None).get(myid)

This would be backwards compatible and would also avoid any name-conflict woes. Of course, I don't like that it's not so readable anymore, but it might be useful.

Another approach is to have get return None instead of an exception:

  foo = Foo.objects.get(myid) or default_object

But I doubt that the whole DoesNotExist exception thing would be thrown away at this stage. Maybe a settings variable can be used to "turn on" this functionality, to keep it backways compatible and let the user choose whether or not they want exceptions or return values. This road is getting a little messy.

A third option is:

  foo = Foo.objects.get_or_none(myid) or default_object

...but the method needs a better name, because it's really starting to lose it's readability, but I like it better than the first one.

comment:4 Changed 8 years ago by dcwatson

The patch adds a "default" kwarg, that returns the value of "default" if it's supplied, and raises the DoesNotExist exception otherwise, like before. So you can do either of the following:

try:
  foo = Foo.objects.get( pk=myid )
except Foo.DoesNotExist, ex:
  foo = None

or

foo = Foo.objects.get( pk=myid, default=None )

I think making get() use the first argument as a primary key lookup is unnecessary with the "pk" shortcut. Also, I'd imagine it would add a lot of ugly special-casing, since get() is really a convenience for filter(), whose positional args (if provided) are expected to be Q objects.

comment:5 Changed 8 years ago by jacob

  • Triage Stage changed from Unreviewed to Accepted

Changed 7 years ago by dcwatson

updated patch against r8194

comment:6 Changed 7 years ago by mtredinnick

  • Triage Stage changed from Accepted to Design decision needed

Dan's going to want to kill me (sorry, Dan!), but we can't commit this as it is. It's backwards incompatible. If somebody has a qs.get(foo=1, default=2) call now, that code will break with this change.

So if this is really, really considered necessary (and the bar just got higher because of this), it need to be a method with a new name. What I don't want to see are 23 comments attached to this ticket suggesting everybody's favorite name for the new pony. Somebody (hi, Dan!) should come up with a good name or two and gather some consensus on django-dev, which should probably include some kind of glimmer of interest from somebody with commit privileges. I'm kind of somewhere around +0/-0 on this. I use the pattern a fair bit myself, but it doesn't worry me too much (it's a three line utility function if I don't want to catch exceptions too often). Since it's introducing a whole new method, it's going to need some support. I know I'm not really helping there, but maybe I'm undervaluing the ticket's utility so I'm not going to wontfix it immediately.

As some consolation, here's an updated patch that at least applies against trunk. So it can be the starting point for modification.

Changed 7 years ago by mtredinnick

Updated patch (just the docs bit changed)

comment:7 Changed 6 years ago by dcwatson

I'm fine with relegating it to utility-function land. It may have been nice to mirror dict.get(), but it's by no means difficult to catch the exception, create a utility function, or even have your own manager get() method. Or perhaps it would be better suited as a get_object method in django.shortcuts. In any case, I don't feel strongly enough about it to carry the torch, so feel free to wontfix it or defer it to 2.0, when we can break stuff with reckless abandon!

comment:8 Changed 6 years ago by kevinh

Can we put this in the django.shortcuts?

please?

def get_object_or_none(klass, *args, **kwargs):
    """
    Uses get() to return an object, or return None if the
    does not exist.
 
    klass may be a Model, Manager, or QuerySet object. All other passed
    arguments and keyword arguments are used in the get() query.
 
    Note: Like with get(), an MultipleObjectsReturned will be raised if more than one
    object is found.
    """
    queryset = _get_queryset(klass)
    try:
        return queryset.get(*args, **kwargs)
    except queryset.model.DoesNotExist:
        return None

comment:9 Changed 5 years ago by mtredinnick

  • Resolution set to wontfix
  • Status changed from new to closed

Due to backwards incompatibility changes, this can't be done. Sorry. :-(

Writing one's own shortcut is easy enough here.

comment:10 Changed 4 years ago by anonymous

  • Severity set to Normal
  • Type set to Uncategorized

This is the sort of thing that would be useful if you ever do a big API break (django 2.0) for instance. - Maybe it would be worth collecting all these somewhere like that, so a future version after an API break could benefit?

comment:11 Changed 2 years ago by bignose

  • Cc bignose added
  • Easy pickings unset
  • Keywords latest earliest added
  • Summary changed from make queryset get() take a default kwarg to make queryset get(), latest(), earliest(), take a default kwarg
  • UI/UX unset

We are looking for the same behaviour, but from the ‘QuerySet.latest’ and friends.

Quite a lot of our code looks like this:

foo_set = Foo.filter(spam="Lorem", beans="Ipsum")
try:
    foo = foo_set.latest()
except Foo.DoesNotExist:
    foo = None
do_more_with(foo)

This would be cleaner (and less prone to bugs) written as:

foo_set = Foo.filter(spam="Lorem", beans="Ipsum")
foo = foo_set.latest(default=None)
do_more_with(foo)

This is a generally-useful feature, and doesn't belong tacked onto every model manager; it should be in the QuerySet API so every model gets it.

Note: See TracTickets for help on using tickets.
Back to Top