Opened 12 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:
- 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.
- 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 , 12 years ago
comment:2 by , 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 , 11 years ago
Cc: | added |
---|
comment:4 by , 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 , 11 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:6 by , 11 years ago
Triage Stage: | Accepted → 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:8 by , 11 years ago
Cc: | added |
---|---|
Has patch: | set |
Version: | 1.5 → master |
Submitted Pull request, with fix, tests and docs.
comment:9 by , 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 , 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 , 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 , 11 years ago
Resolution: | → wontfix |
---|---|
Status: | new → closed |
Closing this as wontfix given no consensus
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.