Proposal for simplifying part of the GCBV API slightly.

Showing 1-6 of 6 messages
Proposal for simplifying part of the GCBV API slightly. Tom Christie 4/22/13 4:15 AM
I'd be interested to know what you folks think of this proposal.

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?

Any opinions?

  Tom

As an aside, is the discussion group considered the correct place for API-changing proposals like this, or should I just raise them straight to the ticket tracker?
Re: Proposal for simplifying part of the GCBV API slightly. Calvin Spealman 4/22/13 4:37 AM


On Apr 22, 2013 7:15 AM, "Tom Christie" <christ...@gmail.com> wrote:
>
> I'd be interested to know what you folks think of this proposal.
>
> 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.

I can think of a few reasons.
- you've changed models or fields internally through migrations, but need to continue supporting old URLs.
- you don't like the internal name to be exposed
- you have different models but want to expose a consistent URL pattern.

> 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?
>
> Any opinions?
>
>   Tom
>
> As an aside, is the discussion group considered the correct place for API-changing proposals like this, or should I just raise them straight to the ticket tracker?
>
> --
> You received this message because you are subscribed to the Google Groups "Django developers" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
> To post to this group, send email to django-d...@googlegroups.com.
> Visit this group at http://groups.google.com/group/django-developers?hl=en.
> For more options, visit https://groups.google.com/groups/opt_out.
>  
>  

Re: Proposal for simplifying part of the GCBV API slightly. Tom Christie 4/22/13 5:33 AM
Hi Calvin,

I can think of a few reasons.
- you've changed models or fields internally through migrations, but need to continue supporting old URLs.
- you don't like the internal name to be exposed
- you have different models but want to expose a consistent URL pattern.

Those attributes control the URL kwargs that are used in the regex, we're not talking about URL query parameters or anything else that's visible to the end-user.  Internal names aren't being exposed anywhere, and there'd be no issue with updating field names and continuing to support the URLs that reference them.

Having said that, you're probably correct that I've overstated point (1) - There might be some circumstances where the developer would prefer to use 'slug' as the regex kwarg, but look up against a differently named model field.  I'd still think that there's a very strong argument to be made that we could consider that a corner case that requires overriding `get_object`, in exchange for having a simpler API for the standard case.

There would of course be other alternatives such as using both `lookup_field` and `lookup_kwarg` with the later defaulting to the same as the former if unset, but I'm not wild about something like that given that the intention of this proposal is to try to slightly slim down an aspect of the API.
Re: Proposal for simplifying part of the GCBV API slightly. Andrew Ingram 4/22/13 6:40 AM
The original use case for pk_url_kwarg and slug_url_kwargs was something like this:

/(?P<slug>[\w-]*)/ - DetailView
/(?P<slug>[\w-]*)/another-resource/ - Scoped ListView
/(?P<slug>[\w-]*)/another-resource/(?P<another_slug>[\w-]*) - Scoped DetailView

In this case, the Scoped ListView and Scoped DetailView would both inherit a mixin that overrides get_queryset(), and the Scope DetailView would just set "slug_url_kwargs = 'another_slug'"

I've used some variant of this pattern on almost every project I've been involved with that utilises class-based views, including some frameworks, so I don't think this edge case is quite as edgy as it might seem at first.

However, I do agree that your simplification is an improvement. I've done a lot with CBVs since I first suggested the URL kwarg overrides, and I think it's better to have less configurable fields and focus instead on having good entry points into customising the classes through method overrides.


Andy

Re: Proposal for simplifying part of the GCBV API slightly. Tom Christie 5/2/13 12:58 AM
I've created ticket #20432 for this proposal.
I don't know if it needs to go to 'design decision needed', but if it gets approved I'll plan to work on it at the DjangoCon sprints, aiming for the 1.6 alpha.

  Tom
Re: Proposal for simplifying part of the GCBV API slightly. Tom Christie 5/12/13 12:05 AM
Right now that ticket's just got a cautious +0 against it.
I still think this would be a small step in the right direction of presenting a slightly simpler GCBV API, but unless there's any further support I'm inclined to give the ticket a few more days and then close it.