Opened 2 years ago

Closed 2 years ago

#20342 closed Cleanup/optimization (wontfix)

Replace CBV lookup arguments with single `lookup_field` argument.

Reported by: tomchristie Owned by: tomchristie
Component: Generic views Version: master
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 Changed 2 years ago by mjtamlyn

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

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 Changed 2 years ago by HM

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 Changed 2 years ago by mjtamlyn

  • Cc marc.tamlyn@… added

comment:4 Changed 2 years ago by tomchristie

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 Changed 2 years ago by mjtamlyn

  • Triage Stage changed from Unreviewed to Accepted

comment:6 Changed 2 years ago by aaugustin

  • Triage Stage changed from Accepted to Unreviewed

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 Changed 2 years ago by aaugustin

  • Triage Stage changed from Unreviewed to Accepted

Whoops, accidental change.

comment:8 Changed 2 years ago by tomchristie

  • Cc tom@… added
  • Has patch set
  • Version changed from 1.5 to master

Submitted Pull request, with fix, tests and docs.

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

comment:9 Changed 2 years ago by anonymous

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 Changed 2 years ago by tomchristie

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 Changed 2 years ago by mjtamlyn

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 Changed 2 years ago by tomchristie

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

Closing this as wontfix given no consensus

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