[Django] #20342: Replace CBV lookup arguments with single `lookup_field` argument.
Django
--------------------------------------+-------------------------
Reporter: tomchristie | Owner: tomchristie
Type: Cleanup/optimization | Status: new
Component: Generic views | Version: 1.5
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+-------------------------
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/20342>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: tomchristie | Owner:
Type: | tomchristie
Cleanup/optimization | Status: new
Component: Generic views | Version: 1.5
Keywords: | Triage Stage:
Has patch: 0 | Unreviewed
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by mjtamlyn):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20342#comment:1>
Django
Reporter: tomchristie | Owner:
Type: | tomchristie
Component: Generic views | Version: 1.5
Keywords: | Triage Stage:
Has patch: 0 | Unreviewed
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Comment (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'}`
--
Ticket URL: <https://code.djangoproject.com/ticket/20342#comment:2>
Django
Reporter: tomchristie | Owner:
Type: | tomchristie
Component: Generic views | Version: 1.5
Keywords: | Triage Stage:
Has patch: 0 | Unreviewed
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
* cc: marc.tamlyn@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/20342#comment:3>
Django
Reporter: tomchristie | Owner:
Type: | tomchristie
Component: Generic views | Version: 1.5
Keywords: | Triage Stage:
Has patch: 0 | Unreviewed
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20342#comment:4>
Django
Reporter: tomchristie | Owner:
Type: | tomchristie
Component: Generic views | Version: 1.5
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by mjtamlyn):
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/20342#comment:5>
Django
Reporter: tomchristie | Owner:
Type: | tomchristie
Component: Generic views | Version: 1.5
Keywords: | Triage Stage:
Has patch: 0 | Unreviewed
Needs tests: 0 | Needs documentation: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by aaugustin):
* stage: Accepted => Unreviewed
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20342#comment:6>
Django
Reporter: tomchristie | Owner:
Type: | tomchristie
Component: Generic views | Version: 1.5
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by aaugustin):
* stage: Unreviewed => Accepted
Comment:
Whoops, accidental change.
--
Ticket URL: <https://code.djangoproject.com/ticket/20342#comment:7>
Django
Reporter: tomchristie | Owner:
Type: | tomchristie
Component: Generic views | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by tomchristie):
* cc: tom@… (added)
* has_patch: 0 => 1
* version: 1.5 => master
Comment:
Submitted Pull request, with fix, tests and docs.
https://github.com/django/django/pull/1205
--
Ticket URL: <https://code.djangoproject.com/ticket/20342#comment:8>
Django
Reporter: tomchristie | Owner:
Type: | tomchristie
Component: Generic views | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20342#comment:9>
Django
Reporter: tomchristie | Owner:
Type: | tomchristie
Component: Generic views | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20342#comment:10>
Django
Reporter: tomchristie | Owner:
Type: | tomchristie
Component: Generic views | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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>/
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20342#comment:11>
Django
Reporter: tomchristie | Owner:
Type: | tomchristie
Component: Generic views | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by tomchristie):
* status: new => closed
* resolution: => wontfix
Comment:
Closing this as wontfix given no consensus
--
Ticket URL: <https://code.djangoproject.com/ticket/20342#comment:12>