Possible deprecation of depth= in select_related

[Marc Tamlyn]

Hi all,

The issue #16855 [1] tracks some unexpected behaviour in the chaining of `Queryset.select_related`. It's been proving rather complex to get a patch for this which works, mainly because of the complexity added by the depth argument. It has been proposed (by Luke Plant) that depth be deprecated (and +1d by several other users and a core dev). With this in mind, I propose that we put the kwarg on the deprecation path adding the warning in 1.5 (so the bug can eventually be fixed in 1.7). As it changes no functionality in this release, I don't see any reason to hold this for 1.6 (and thereby delay the fix until 1.8).

If anyone has any major objections to the deprecation of depth, you should shout now. If there are no objections and people think it's ok to push this deprecation in now, then I'll get a patch done on Monday.

Thanks,

Marc

[1] https://code.djangoproject.com/ticket/16855

[Jacob Kaplan-Moss]

On Fri, Nov 2, 2012 at 9:29 AM, Marc Tamlyn <marc....@gmail.com> wrote:
> If anyone has any major objections to the deprecation of depth, you should
> shout now. If there are no objections and people think it's ok to push this
> deprecation in now, then I'll get a patch done on Monday.

No objections here. Depth was a simple protection we added before we
got select_related(*fields); the *fields form is a ton more useful. +1
on deprecating depth.

Jacob

[Russell Keith-Magee]

Sounds good to me too. +1.

Russ %-)

[Marc Tamlyn]

It seems there had been a little confusion as to exactly what the initial proposal was. I had assumed it was just about removing the `depth` argument but it was still possible to pass *no* arguments to get the implicit "follow everything" behaviour. In fact Luke was suggesting that we remove this behaviour as well, requiring the user to pass in some field names. This has various knock on effects within Django (mostly in the admin), but they're not particularly difficult to fix. My concern is that I've seen "blank" select related calls quite often in the wild (although never seen depth=), so it may break a few more things. Then again, blank select related calls are pretty risky, and although it's not documented are actually equivalent to `depth=5`. I'm not really against deprecating this but I thought it should be made clear exactly what the deprecation was (and we should get this sorted out for 1.5).

Marc

[ptone]

On Wednesday, November 7, 2012 1:48:09 AM UTC-8, Marc Tamlyn wrote:

It seems there had been a little confusion as to exactly what the initial proposal was. I had assumed it was just about removing the `depth` argument but it was still possible to pass *no* arguments to get the implicit "follow everything" behaviour. In fact Luke was suggesting that we remove this behaviour as well, requiring the user to pass in some field names. This has various knock on effects within Django (mostly in the admin), but they're not particularly difficult to fix. My concern is that I've seen "blank" select related calls quite often in the wild (although never seen depth=), so it may break a few more things. Then again, blank select related calls are pretty risky, and although it's not documented are actually equivalent to `depth=5`. I'm not really against deprecating this but I thought it should be made clear exactly what the deprecation was (and we should get this sorted out for 1.5).

Marc

I agree deprecating select_related() in favor of select_related(*fields) is a whole 'nother ball of wax over just deprecating depth kwarg (which has now been done).

While using *fields explicitly may be the better way to go in most cases, and we can document that more strongly - it feels like eliminating the historically simple version, which can still be useful in simple, shallow cases goes a bit far - but I'm more of a -0 than -1 on it.

-Preston