Re: [Django] #20342: Replace CBV lookup arguments with single `lookup_field` argument.

[Django]

#20342: Replace CBV lookup arguments with single `lookup_field` argument.
-+-
 Reporter:  tomchristie  |Owner:
 Type:   |  tomchristie
  Cleanup/optimization   |   Status:  closed
Component:  Generic views|  Version:  master
 Severity:  Normal   |   Resolution:  wontfix
 Keywords:   | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/069.d3ad5ae50302e560edb5b0e68956f044%40djangoproject.com?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [Django] #20342: Replace CBV lookup arguments with single `lookup_field` argument.

[Django]

#20342: Replace CBV lookup arguments with single `lookup_field` argument.
-+-
 Reporter:  tomchristie  |Owner:
 Type:   |  tomchristie
  Cleanup/optimization   |   Status:  new
Component:  Generic views|  Version:  master
 Severity:  Normal   |   Resolution:
 Keywords:   | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  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:

 {{{
 //
 ///
 
 }}}

 I think that is clearer in the `urls.py` than

 {{{
 //
 ///
 
 }}}

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/069.bb0dfa6ae26a270b3755fa4d5a591ba9%40djangoproject.com?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [Django] #20342: Replace CBV lookup arguments with single `lookup_field` argument.

[Django]

#20342: Replace CBV lookup arguments with single `lookup_field` argument.
-+-
 Reporter:  tomchristie  |Owner:
 Type:   |  tomchristie
  Cleanup/optimization   |   Status:  new
Component:  Generic views|  Version:  master
 Severity:  Normal   |   Resolution:
 Keywords:   | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/069.5c7086f85ce813339d6124e6615f3528%40djangoproject.com?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [Django] #20342: Replace CBV lookup arguments with single `lookup_field` argument.

[Django]

#20342: Replace CBV lookup arguments with single `lookup_field` argument.
-+-
 Reporter:  tomchristie  |Owner:
 Type:   |  tomchristie
  Cleanup/optimization   |   Status:  new
Component:  Generic views|  Version:  master
 Severity:  Normal   |   Resolution:
 Keywords:   | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/069.1f816dbe31a31d13f85bb1335f7ca02e%40djangoproject.com?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [Django] #20342: Replace CBV lookup arguments with single `lookup_field` argument.

[Django]

#20342: Replace CBV lookup arguments with single `lookup_field` argument.
-+-
 Reporter:  tomchristie  |Owner:
 Type:   |  tomchristie
  Cleanup/optimization   |   Status:  new
Component:  Generic views|  Version:  master
 Severity:  Normal   |   Resolution:
 Keywords:   | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/069.8c67ecd48c9a140cfa68fd3e7afbaf33%40djangoproject.com?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [Django] #20342: Replace CBV lookup arguments with single `lookup_field` argument.

[Django]

#20342: Replace CBV lookup arguments with single `lookup_field` argument.
-+-
 Reporter:  tomchristie  |Owner:
 Type:   |  tomchristie
  Cleanup/optimization   |   Status:  new
Component:  Generic views|  Version:  1.5
 Severity:  Normal   |   Resolution:
 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 aaugustin):

 * stage:  Unreviewed => Accepted


Comment:

 Whoops, accidental change.

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [Django] #20342: Replace CBV lookup arguments with single `lookup_field` argument.

[Django]

#20342: Replace CBV lookup arguments with single `lookup_field` argument.
-+-
 Reporter:  tomchristie  |Owner:
 Type:   |  tomchristie
  Cleanup/optimization   |   Status:  new
Component:  Generic views|  Version:  1.5
 Severity:  Normal   |   Resolution:
 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 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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [Django] #20342: Replace CBV lookup arguments with single `lookup_field` argument.

[Django]

#20342: Replace CBV lookup arguments with single `lookup_field` argument.
-+-
 Reporter:  tomchristie  |Owner:
 Type:   |  tomchristie
  Cleanup/optimization   |   Status:  new
Component:  Generic views|  Version:  1.5
 Severity:  Normal   |   Resolution:
 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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [Django] #20342: Replace CBV lookup arguments with single `lookup_field` argument.

[Django]

#20342: Replace CBV lookup arguments with single `lookup_field` argument.
-+-
 Reporter:  tomchristie  |Owner:
 Type:   |  tomchristie
  Cleanup/optimization   |   Status:  new
Component:  Generic views|  Version:  1.5
 Severity:  Normal   |   Resolution:
 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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [Django] #20342: Replace CBV lookup arguments with single `lookup_field` argument.

[Django]

#20342: Replace CBV lookup arguments with single `lookup_field` argument.
-+-
 Reporter:  tomchristie  |Owner:
 Type:   |  tomchristie
  Cleanup/optimization   |   Status:  new
Component:  Generic views|  Version:  1.5
 Severity:  Normal   |   Resolution:
 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):

 * cc: marc.tamlyn@… (added)


-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [Django] #20342: Replace CBV lookup arguments with single `lookup_field` argument.

[Django]

#20342: Replace CBV lookup arguments with single `lookup_field` argument.
-+-
 Reporter:  tomchristie  |Owner:
 Type:   |  tomchristie
  Cleanup/optimization   |   Status:  new
Component:  Generic views|  Version:  1.5
 Severity:  Normal   |   Resolution:
 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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [Django] #20342: Replace CBV lookup arguments with single `lookup_field` argument.

[Django]

#20342: Replace CBV lookup arguments with single `lookup_field` argument.
-+-
 Reporter:  tomchristie  |Owner:
 Type:   |  tomchristie
  Cleanup/optimization   |   Status:  new
Component:  Generic views|  Version:  1.5
 Severity:  Normal   |   Resolution:
 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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

[Django] #20342: Replace CBV lookup arguments with single `lookup_field` argument.

[Django]

#20342: Replace CBV lookup arguments with single `lookup_field` argument.
--+-
 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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.