Opened 11 years ago

Closed 11 years ago

#20342 closed Cleanup/optimization (wontfix)

Replace CBV lookup arguments with single `lookup_field` argument.

Reported by: Tom Christie Owned by: Tom Christie
Component: Generic views Version: dev
Severity: Normal Keywords:
Cc: marc.tamlyn@…, tom@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

As discussed on this thread: https://groups.google.com/forum/?fromgroups=#!topic/django-developers/ht6Xq2ytPe4.

SingleObjectMixin currently exposes these three attributes and one method all dealing with queryset filter arguments...

  • pk_url_kwarg = 'pk'
  • slug_field = 'slug'
  • slug_url_kwarg = 'slug'
  • get_slug_field()

I was wondering if it would be worth considering simplifying the API somewhat, by moving those three attributes, and that method, into a PendingDeprecation state, and replacing their usage with a single attribute instead, that is used both as the URL kwarg, and as the queryset filter.

  • lookup_field = 'pk'

That'd (eventually) lead to a simpler get_object implementation internally, and (immediately) present a slightly nicer, simpler API.

It'd be marginally different in that slug based lookups would require an explicit lookup_field = 'slug' to be added to the view,
and also in that it wouldn't handle the case of slug_field being set to a different value from slug_url_kwarg.

Personally I don't see the later being an issue as:

  1. It's not clear to me why you'd ever *need* to use a URL kwarg that's not the same as the filter arg.
  2. If it's really something you need to do, then overriding get_object is still really simple, as well as being nice and explicit...

get_object(self, queryset):

if queryset is None:

queryset = self.get_queryset()

return get_object_or_404(queryset, ...) # whatever custom lookup behavior you need.

To me, the main trade-off seems to be - Is the resulting API enough of an improvement to be worth the change?

I'm more than happy to take on the work, but I guess this ticket might need to go through the design decision needed stage?

Change History (12)

comment:1 by Marc Tamlyn, 11 years ago

Gonna put in a +0 on this - it's a nicer API but I'm not sure it's worth changing things for. I've never had much problem just using something else as "slug", and it's nice that both "slug" and "pk" work automatically without any more configuration.

comment:2 by HM, 11 years ago

I usually wind up with long, complex urls and looking up more than one thing actually, so would prefer something like:

# key: kwarg, value: model field

lookups = {
    'pk': 'pk', 
    'slug': 'slug', 
    'otherkey': 'othermodel__serialno', 
    'owner': 'owner__username', 
    'year': 'year_published'
}

and then either a separate function to set them, run during dispatch(), or one dynamic function per item.

The default lookups would then be {'pk': 'pk', 'slug': 'slug'}


comment:3 by Marc Tamlyn, 11 years ago

Cc: marc.tamlyn@… added

comment:4 by Tom Christie, 11 years ago

If this gets some enthusiasm, then i think it'd be worthwhile, otherwise I'm also happy to WONTFIX this, on the assumption that the current lookup API is good enough.

Not keen on the dict based approach, if we're going to refactor the API I think it's only worthwhile if we're refactoring for simplicity.

comment:5 by Marc Tamlyn, 11 years ago

Triage Stage: UnreviewedAccepted

comment:6 by Aymeric Augustin, 11 years ago

Triage Stage: AcceptedUnreviewed

Tom: after discussing with mjtamlyn at the DjangoCon sprints, as well as reading Andrew Ingram's comments on the mailing list, I'm rather in favor of simplifying the API.

The solution proposed in comment 2, as well as many others, can be achieved through subclassing.

comment:7 by Aymeric Augustin, 11 years ago

Triage Stage: UnreviewedAccepted

Whoops, accidental change.

comment:8 by Tom Christie, 11 years ago

Cc: tom@… added
Has patch: set
Version: 1.5master

Submitted Pull request, with fix, tests and docs.

https://github.com/django/django/pull/1205

comment:9 by anonymous, 11 years ago

I like simplifying this, but I don't think it's appropriate to force that the urlpattern name matches the model field name. We need two parameters here -- one is the urlconf name, the other is the model field. lookup_url_field and lookup_model_field, maybe. Assuming that the urlconf matches the model is inappropriate coupling.

comment:10 by Tom Christie, 11 years ago

I don't agree that the loose coupling between URL conf and models is an issue. Specifying url kwargs that correspond correctly to the relevant model fields is a good thing. The arguments then make more sense when used in the corresponding reverse lookups. Not doing so creates confusion, eg if you have username based user lookup, but refer in the URL conf to the field as slug, then it's not obvious if the URLconf is in error, or if the disparity is intended.

comment:11 by Marc Tamlyn, 11 years ago

Here's an example where it would be more confusing to *not* use a different name:

/<publisher>/
/<publisher>/<author>/
/<publisher>/<author>/<book>/

I think that is clearer in the urls.py than

/<slug>/
/<publisher>/<slug>/
/<publisher>/<author>/<slug>/

comment:12 by Tom Christie, 11 years ago

Resolution: wontfix
Status: newclosed

Closing this as wontfix given no consensus

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