RE: Custom Chainable QuerySets (#20625)

2013-07-24 Thread Kääriäinen Anssi 
Same pull request at https://github.com/django/django/pull/1328. Seems like it 
is still getting some review & update activity. I am planning on doing a final 
review on commit, but judging by the amount of reviews done already I think 
this one will be very polished by Friday.

From: django-developers@googlegroups.com [django-developers@googlegroups.com] 
On Behalf Of Aymeric Augustin [aymeric.augus...@polytechnique.org]
Sent: Wednesday, July 24, 2013 21:39
To: django-developers@googlegroups.com
Subject: Re: Custom Chainable QuerySets (#20625)

On 24 juil. 2013, at 13:53, Anssi Kääriäinen  wrote:

> I will commit the patch on Friday. If somebody wants more time to review the 
> patch, just ask and I will defer the commit to later date.

Where's the version of the patch you're ready to commit?

--
Aymeric.




--
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-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
For more options, visit https://groups.google.com/groups/opt_out.


-- 
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-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
For more options, visit https://groups.google.com/groups/opt_out.

Re: Custom Chainable QuerySets (#20625)

2013-07-24 Thread Aymeric Augustin 
On 24 juil. 2013, at 13:53, Anssi Kääriäinen  wrote:

> I will commit the patch on Friday. If somebody wants more time to review the 
> patch, just ask and I will defer the commit to later date.

Where's the version of the patch you're ready to commit?

-- 
Aymeric.




-- 
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-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
For more options, visit https://groups.google.com/groups/opt_out.

Re: Custom Chainable QuerySets (#20625)

2013-07-24 Thread Anssi Kääriäinen 
On Tuesday, July 23, 2013 12:39:13 PM UTC+3, Loic Bistuer wrote:
>
> I thought of a third option that I think strikes the right balance between 
> convenience and flexibility. 
>
> Please see the following comment: 
>
> https://github.com/django/django/pull/1328#issuecomment-21400832 
>
>
I will go with the above mentioned approach.

This means that there will be MyQuerySet.as_manager() method. The method is 
just a convenience shortcut for Manager.from_queryset(MyQuerySet)().

If I understand correctly the need for .as_manager() is the only issue 
which is still under discussion. There were two core -0, one +1 (me). There 
were other core members who have reviewed the patch but didn't raise any 
opinion on the .as_manager() method.

Strongest opposition was about the coupling of QuerySet to Manager. The 
current form of .as_manager() doesn't have any strong coupling, and is very 
unlikely to cause maintenance problems. The method is two lines long, and 
those are simple lines. So I feel this is mainly an API issue now.

The .as_manager() could be added separately later on after further 
discussion, but this means a fair bit of extra work (rewrite of the docs & 
tests of the current patch, then a new rewrite for the .as_manager() 
patch), so lets avoid that if possible.

I don't feel that strongly about the method. My gut feeling is that it will 
be convenient. If I get a -1 from somebody, then the method will be gone, 
no questions asked. If I don't get any -1 I will commit the patch on 
Friday. If somebody wants more time to review the patch, just ask and I 
will defer the commit to later date.

 - Anssi

-- 
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-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
For more options, visit https://groups.google.com/groups/opt_out.

Re: Custom Chainable QuerySets (#20625)

2013-07-23 Thread Loic Bistuer 
I thought of a third option that I think strikes the right balance between 
convenience and flexibility.

Please see the following comment:

https://github.com/django/django/pull/1328#issuecomment-21400832

-- 
Loic

-- 
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-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
For more options, visit https://groups.google.com/groups/opt_out.

Re: Custom Chainable QuerySets (#20625)

2013-07-22 Thread Loic Bistuer 

On Jul 23, 2013, at 3:17 AM, Aymeric Augustin 
 wrote:

> Either way the factory should receive the QuerySet in argument as well as any 
> other parameters required to create the custom Manager appropriately.

I missed the "other parameters required" part.

Please note that this is only needed for the specific case where you want a 
`Manager` *automatically* generated from both a custom `QuerySet` and a custom 
base `Manager` that requires `__init__()` arguments.

I see this `CustomQuerySet.as_manager()` feature as "this is the filtering I'm 
going to need, make sure I can also call it from the manager", which should 
cover the vast majority of the cases.

Since `as_manager()` is just a thin wrapper around 
`Manager.create_from_queryset()`, I would rather do that:

BaseCustomManager(Manager):
def __init__(self, custom_arg):
...

CustomManager = BaseCustomManager.create_from_queryset(CustomQuerySet)

manager = CustomManager('custom')

And of course we can still use the old fashioned way when absolute control is 
needed.

Anyway if we *must* support the `__init__()` arguments, it will be an issue for 
`as_manager(base_class=None)` because `base_class` needs to be a `kwarg` in 
order to be optional and we can't accept `*args` after a `kwarg`.

We could:
- Fish for `base_class` in `**kwargs` but then we can't do 
`CustomQuerySet.as_manager(CustomManager)` anymore.
- Support only `**kwargs` (and not `*args`) to pass onto  `Manager.__init__()`.
- Change the signature to `as_manager(base_class=None, args=None, kwargs=None)`.

Personally, I wouldn't do anything for the `__init__()` arguments as I consider 
that to be out of scope for the  `as_manager()` convenience method.

Of course `from_queryset()` doesn't have this issue since the first argument is 
never optional.

-- 
Loic

-- 
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-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
For more options, visit https://groups.google.com/groups/opt_out.

Re: Custom Chainable QuerySets (#20625)

2013-07-22 Thread Loic Bistuer 
I'm not a big fan of TIMTOWTDI so I don't think we should have both 
`Manager.from_queryset()` and `QuerySet.as_manager()`.

The following commit moved all the logic that was on `QuerySet` to the 
`Manager` class and implemented the `from_queryset()` API:

https://github.com/loic/django/commit/e53ce4b

The resulting API is:

from django.db.models import Manager
with_default_manager = Manager.from_queryset(CustomQuerySet)
with_custom_manager = CustomManager.from_queryset(CustomQuerySet)

The following commit builds on the previous one and removes `from_queryset()` 
in favor of  `as_manager()` while keeping most of the logic on the `Manager` 
class:

https://github.com/loic/django/commit/57bedb6

The resulting API is:

with_default_manager = CustomQuerySet.as_manager()
with_custom_manager = CustomQuerySet.as_manager(CustomManager)

Anssi summarized the pros and cons of each approach on 
https://github.com/django/django/pull/1328#issuecomment-21365845.

I'm not gonna try to weight in any further for one API or the other; both are 
implemented and I'll update the docs as soon as a decision is taken.

-- 
Loic

On Jul 23, 2013, at 3:17 AM, Aymeric Augustin 
 wrote:

> If you really want the short and sweet API, I can listen for an argument to 
> implement it as follows:
> 
> class QuerySet:
>def as_manager(self, *args, **kwargs):
># imported here because this method creates a circular dependency
># between Manager  and QuerySet
>from django.db.models import Manager
>return Manager.from_queryset(self, *args, **kwargs)

-- 
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-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
For more options, visit https://groups.google.com/groups/opt_out.

Re: Custom Chainable QuerySets (#20625)

2013-07-22 Thread Aymeric Augustin 
On 22 juil. 2013, at 17:31, Loic Bistuer  wrote:

> The goal is to make it painfully easy to get a `Manager` from a custom 
> `QuerySet` and for most cases, having the factory on `Manager` instead of 
> `QuerySet` results in a more verbose syntax and requires an unnecessary 
> import.

Objects with strong knowledge of one another are an OO design anti-pattern, see 
, linked from 
. The QuerySet class is already 
quite a beast; let's keep its responsibilities clearly delineated. A sound 
design will save us pain in the future and is well worth a few extra keystrokes.

For example CBVs are a similar case: Django went for MyView.as_view(**kwargs), 
which wasn't the shortest syntax, but allowed greater flexibility.

> In any case, a factory is better than the `Manager(MyQuerySet)` syntax (may 
> it be through __new__, self.__class__, __getattr__ or __getattribute__) 
> because it lets us create the classes ahead of time and it works well with 
> inheritance.

Yes, that sounds right.

I see two options for defining this factory:
- as a class method of the Manager class: this is the most obvious way to write 
a factory;
- as a standalone function: this shouldn't be necessary here, as far as I can 
tell.

Either way the factory should receive the QuerySet in argument as well as any 
other parameters required to create the custom Manager appropriately.

If you really want the short and sweet API, I can listen for an argument to 
implement it as follows:

class QuerySet:
def as_manager(self, *args, **kwargs):
# imported here because this method creates a circular dependency
# between Manager  and QuerySet
from django.db.models import Manager
return Manager.from_queryset(self, *args, **kwargs)

(untested, I just typed that in my mailer)

This alleviates my concerns about the circular dependency — it's fairly limited 
and controlled — but the natural location of a Manager factory is still on the 
Manager class… Make that a -0…

-- 
Aymeric.



-- 
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-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
For more options, visit https://groups.google.com/groups/opt_out.

Re: Custom Chainable QuerySets (#20625)

2013-07-22 Thread Loic Bistuer 
I would like to point out that using both a custom `Manager` *and* a custom 
`QuerySet` is an edge case. After all, you can already do that if you want to.

The goal is to make it painfully easy to get a `Manager` from a custom 
`QuerySet` and for most cases, having the factory on `Manager` instead of 
`QuerySet` results in a more verbose syntax and requires an unnecessary import.

In any case, a factory is better than the `Manager(MyQuerySet)` syntax (may it 
be through __new__, self.__class__, __getattr__ or __getattribute__) because it 
lets us create the classes ahead of time and it works well with inheritance.

-- 
Loic

On Jul 22, 2013, at 9:44 PM, Shai Berger  wrote:

> On Monday 22 July 2013 13:25:38 Anssi Kääriäinen wrote:
>> On Monday, July 22, 2013 1:16:04 PM UTC+3, Loic Bistuer wrote:
>>> On Jul 22, 2013, at 4:38 PM, Chris Wilson
>>> >
>>> 
>>> wrote:
 I think that's very true. How about this?
 
> class MyQuerySet(models.QuerySet):
> def published(self):
> return self.filter(published_date__lte=now())
> 
> class MyModel(models.Model):
> published_date = models.DateTimeField()
> 
> objects = CustomManager(MyQuerySet)
>>> 
>>> That was my original proposal; the first 20 comments on the ticket and
>>> one nearly complete implementation are based on this idea.
>>> 
>>> That approach has the downside of requiring the use of `__getattr__` or
>>> `__getattribute__` which is IMO much more of a hack than a class factory,
>>> especially with backward compatibility in mind.
>> 
>> The Manager(MyQuerySet) approach might actually work with dynamically
>> created methods, too. I believe __new__ can be used for this, or maybe one
>> could just create a dynamic class of self.__class__ in Manager.__init__ and
>> assign it back to self.__class__. If this way works it seems cleaner than
>> the as_manager() way.
>> 
> +1 -- except I would use Aymeric's syntax Manager.from_queryset(qset_class), 
> where from_queryset is a classmethod; this, then, also gives a very elegant 
> solution to manager-only methods:
> 
> class MyQuerySet(QuerySet):
>   pass
> 
> class MyManager(Manager):
>   def manager_only_method(self):
>   pass
> 
> class MyModel(Model):
>   objects = MyManager.from_queryset(MyQuerySet)
> 
> The reason I'd prefer this over playing with __new__ and __init__ is just 
> because it looks a lot less like black magic. Functionally, I think you can 
> get pretty much the same either way.

-- 
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-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
For more options, visit https://groups.google.com/groups/opt_out.

Re: Custom Chainable QuerySets (#20625)

2013-07-22 Thread Shai Berger 
On Monday 22 July 2013 13:25:38 Anssi Kääriäinen wrote:
> On Monday, July 22, 2013 1:16:04 PM UTC+3, Loic Bistuer wrote:
> > On Jul 22, 2013, at 4:38 PM, Chris Wilson
> > >
> > 
> > wrote:
> > > I think that's very true. How about this?
> > > 
> > >>  class MyQuerySet(models.QuerySet):
> > >>  def published(self):
> > >>  return self.filter(published_date__lte=now())
> > >>  
> > >>  class MyModel(models.Model):
> > >>  published_date = models.DateTimeField()
> > >>  
> > >>  objects = CustomManager(MyQuerySet)
> > 
> > That was my original proposal; the first 20 comments on the ticket and
> > one nearly complete implementation are based on this idea.
> > 
> > That approach has the downside of requiring the use of `__getattr__` or
> > `__getattribute__` which is IMO much more of a hack than a class factory,
> > especially with backward compatibility in mind.
> 
> The Manager(MyQuerySet) approach might actually work with dynamically
> created methods, too. I believe __new__ can be used for this, or maybe one
> could just create a dynamic class of self.__class__ in Manager.__init__ and
> assign it back to self.__class__. If this way works it seems cleaner than
> the as_manager() way.
> 
+1 -- except I would use Aymeric's syntax Manager.from_queryset(qset_class), 
where from_queryset is a classmethod; this, then, also gives a very elegant 
solution to manager-only methods:

class MyQuerySet(QuerySet):
pass

class MyManager(Manager):
def manager_only_method(self):
pass

class MyModel(Model):
objects = MyManager.from_queryset(MyQuerySet)

The reason I'd prefer this over playing with __new__ and __init__ is just 
because it looks a lot less like black magic. Functionally, I think you can 
get pretty much the same either way.

Shai.

-- 
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-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
For more options, visit https://groups.google.com/groups/opt_out.

Re: Custom Chainable QuerySets (#20625)

2013-07-22 Thread Chris Wilson 

Hi Aymeric,

On Mon, 22 Jul 2013, Aymeric Augustin wrote:

The idea of `QuerySet .as_manager()` sounded sane to me at 
first—especially in the light of the failure of various other 
attempts—but it creates a circular dependency between `Manager` and 
`QuerySet`. Creating the `Manager` class now requires the `QuerySet` 
class, and `QuerySet` needs a reference to `Manager` for `as_manager()`. 
Is it possible to untangle this by writing a 
`Manager.from_queryset(queryset_class)` method instead?


Possibly. Is the dependency at import time? If so, you could import 
manager inside the QuerySet.as_manager() method.



QuerySet shouldn't even know what a Manager is!


I think that's very true. How about this?


  class MyQuerySet(models.QuerySet):
      def published(self):
      return self.filter(published_date__lte=now())

  class MyModel(models.Model):
      published_date = models.DateTimeField()

      objects = CustomManager(MyQuerySet)


Cheers, Chris.
--
Aptivate | http://www.aptivate.org | Phone: +44 1223 967 838
Citylife House, Sturton Street, Cambridge, CB1 2QF, UK

Aptivate is a not-for-profit company registered in England and Wales
with company number 04980791.

--
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-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
For more options, visit https://groups.google.com/groups/opt_out.

Re: Custom Chainable QuerySets (#20625)

2013-07-22 Thread Aymeric Augustin 
On 22 juil. 2013, at 12:16, Loic Bistuer  wrote:

> That was my original proposal; the first 20 comments on the ticket and one 
> nearly complete implementation are based on this idea.

Oh, sorry. I dived into the pull request without reading the ticket first.

I'm really trying to help this ticket move forward (in the best possible way 
for Django), not to hinder it, so please ignore me if I'm rehashing things that 
have already been discussed before.

I'll get back to this thread if I can block a few more hours to review the 
situation in greater detail.

-- 
Aymeric.




-- 
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-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
For more options, visit https://groups.google.com/groups/opt_out.

Re: Custom Chainable QuerySets (#20625)

2013-07-22 Thread Anssi Kääriäinen 
On Monday, July 22, 2013 1:16:04 PM UTC+3, Loic Bistuer wrote:
>
> On Jul 22, 2013, at 4:38 PM, Chris Wilson > 
> wrote: 
>
> > I think that's very true. How about this? 
> > 
> >>  class MyQuerySet(models.QuerySet): 
> >>  def published(self): 
> >>  return self.filter(published_date__lte=now()) 
> >> 
> >>  class MyModel(models.Model): 
> >>  published_date = models.DateTimeField() 
> >> 
> >>  objects = CustomManager(MyQuerySet) 
>
> That was my original proposal; the first 20 comments on the ticket and one 
> nearly complete implementation are based on this idea. 
>
> That approach has the downside of requiring the use of `__getattr__` or 
> `__getattribute__` which is IMO much more of a hack than a class factory, 
> especially with backward compatibility in mind.
>

The Manager(MyQuerySet) approach might actually work with dynamically 
created methods, too. I believe __new__ can be used for this, or maybe one 
could just create a dynamic class of self.__class__ in Manager.__init__ and 
assign it back to self.__class__. If this way works it seems cleaner than 
the as_manager() way.

 - Anssi

-- 
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-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
For more options, visit https://groups.google.com/groups/opt_out.

Re: Custom Chainable QuerySets (#20625)

2013-07-22 Thread Loic Bistuer 
On Jul 22, 2013, at 4:38 PM, Chris Wilson  wrote:

> I think that's very true. How about this?
> 
>>  class MyQuerySet(models.QuerySet):
>>  def published(self):
>>  return self.filter(published_date__lte=now())
>> 
>>  class MyModel(models.Model):
>>  published_date = models.DateTimeField()
>> 
>>  objects = CustomManager(MyQuerySet)

That was my original proposal; the first 20 comments on the ticket and one 
nearly complete implementation are based on this idea.

That approach has the downside of requiring the use of `__getattr__` or 
`__getattribute__` which is IMO much more of a hack than a class factory, 
especially with backward compatibility in mind.

On Jul 22, 2013, at 4:32 PM, Aymeric Augustin 
 wrote:

> The idea of `QuerySet .as_manager()` sounded sane to me at first—especially 
> in the light of the failure of various other attempts—but it creates a 
> circular dependency between `Manager` and `QuerySet`. Creating the `Manager` 
> class now requires the `QuerySet` class, and `QuerySet` needs a reference to 
> `Manager` for `as_manager()`.

We are trying to blur the line between `QuerySet` and `Manager`, or to quote 
you, tackle the "Great QuerySet - Manager Unification". I see this circular 
dependency as an implementation detail due to the fact that we have to deal 
with backward compatibility and we can't just ditch one class or the other.

That said, I willing to try moving some of the code back onto `Manager`.

-- 
Loic

-- 
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-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
For more options, visit https://groups.google.com/groups/opt_out.

Re: Custom Chainable QuerySets (#20625)

2013-07-22 Thread Aymeric Augustin 
The idea of `QuerySet .as_manager()` sounded sane to me at first—especially in 
the light of the failure of various other attempts—but it creates a circular 
dependency between `Manager` and `QuerySet`. Creating the `Manager` class now 
requires the `QuerySet` class, and `QuerySet` needs a reference to `Manager` 
for `as_manager()`.

Is it possible to untangle this by writing a 
`Manager.from_queryset(queryset_class)` method instead? Or at least to put the 
code that creates a Manager from a QuerySet inside the Manager base class? 
QuerySet shouldn't even know what a Manager is!

I also dislike the two ad-hoc APIs spilt over the QuerySet class: the `manager` 
flag on methods and the `base_manager_class` class attribute. I'd really like 
to keep the API self-contained within the `as_manager()` or `from_queryset()` 
method.

I had started a detailed review, but I'm now thinking we should address these 
design decisions first.

-- 
Aymeric.



On 22 juil. 2013, at 10:27, Anssi Kääriäinen  wrote:

> Ticket #20625 deals with the problem of writing custom QuerySet methods. The 
> problem is that one needs to write some boilerplate code to have methods 
> available on both the Manager and the QuerySet. The ticket has a patch for 
> having custom QuerySet methods automatically available on the model's 
> Manager, too.
> 
> The reason for this post is that different ideas for implementing chainable 
> manager/queryset methods have been proposed multiple times. So, I want to 
> make sure we agree on the approach.
> 
> The API idea in #20625 is simple:
> 
> class MyQuerySet(models.QuerySet):
> def published(self):
> return self.filter(published_date__lte=now())
> 
> class MyModel(models.Model):
> published_date = models.DateTimeField()
> 
> objects = MyQuerySet.as_manager()
> 
> The manager created by as_manager() will automatically have a published() 
> method available. The method is created dynamically, and is effectively this:
> 
> def published(self, *args, **kwargs):
> getattr(self.get_query_set(), 'published')(*args, **kwargs)
> 
> The pull request contains more details. Pull request is available from 
> https://github.com/django/django/pull/1328, ticket is 
> https://code.djangoproject.com/ticket/20625.
> 
> The other proposed approaches for chainable manager methods usually use 
> overridden __getattr__() on either manager or queryset and memoizes the other 
> part's class. The __getattr__ then delegates calls to the memoized class. 
> This approach has problems with super() calls and dynamic inspection in pdb. 
> Both of those should work with the proposed approach as the created Manager 
> class really has the methods available.
> 
> Another somewhat common idea is to make Manager a QuerySet subclass and thus 
> avoid this whole Manager/QuerySet split problem. I agree on this idea, but 
> the problem is that this seems to be really hard to do in a way that is even 
> remotely backwards compatible. I tried a couple of different approaches and 
> failed miserably. If somebody has a concrete idea of how to do this, now is a 
> good time to present it.
> 
> It should be noted that the proposed patch doesn't prevent making Manager a 
> QuerySet subclass later on.
> 
> I am planning to do a final review & commit the patch soonish (likely this 
> week).
> 
>  - Anssi
> 
> -- 
> 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-developers+unsubscr...@googlegroups.com.
> To post to this group, send email to django-developers@googlegroups.com.
> Visit this group at http://groups.google.com/group/django-developers.
> For more options, visit https://groups.google.com/groups/opt_out.
>  
>  

-- 
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-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
For more options, visit https://groups.google.com/groups/opt_out.

Custom Chainable QuerySets (#20625)

2013-07-22 Thread Anssi Kääriäinen 
Ticket #20625 deals with the problem of writing custom QuerySet methods. 
The problem is that one needs to write some boilerplate code to have 
methods available on both the Manager and the QuerySet. The ticket has a 
patch for having custom QuerySet methods automatically available on the 
model's Manager, too.

The reason for this post is that different ideas for implementing chainable 
manager/queryset methods have been proposed multiple times. So, I want to 
make sure we agree on the approach.

The API idea in #20625 is simple:

class MyQuerySet(models.QuerySet):
def published(self):
return self.filter(published_date__lte=now())

class MyModel(models.Model):
published_date = models.DateTimeField()

objects = MyQuerySet.as_manager()

The manager created by as_manager() will automatically have a published() 
method available. The method is created dynamically, and is effectively 
this:

def published(self, *args, **kwargs):
getattr(self.get_query_set(), 'published')(*args, **kwargs)

The pull request contains more details. Pull request is available from 
https://github.com/django/django/pull/1328, ticket is 
https://code.djangoproject.com/ticket/20625.

The other proposed approaches for chainable manager methods usually use 
overridden __getattr__() on either manager or queryset and memoizes the 
other part's class. The __getattr__ then delegates calls to the memoized 
class. This approach has problems with super() calls and dynamic inspection 
in pdb. Both of those should work with the proposed approach as the created 
Manager class really has the methods available.

Another somewhat common idea is to make Manager a QuerySet subclass and 
thus avoid this whole Manager/QuerySet split problem. I agree on this idea, 
but the problem is that this seems to be really hard to do in a way that is 
even remotely backwards compatible. I tried a couple of different 
approaches and failed miserably. If somebody has a concrete idea of how to 
do this, now is a good time to present it.

It should be noted that the proposed patch doesn't prevent making Manager a 
QuerySet subclass later on.

I am planning to do a final review & commit the patch soonish (likely this 
week).

 - Anssi

-- 
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-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
For more options, visit https://groups.google.com/groups/opt_out.