On Jan 12, 2:24 pm, Zachary Voase <[email protected]> wrote: > I strongly feel that switching around managers like that would be an > example of overengineering.
I tend to over-engineer things. It is easy to add the manager switching later on if needed. And it is easy to switch managers by just changing qs.manager if really needed. So, I agree on leaving that feature out. > Here's the change I > propose:https://github.com/zacharyvoase/django/compare/master...feature-query... > > It's very light for now, I'm not sure if there are other corner cases > where it breaks, but it's the simplest thing that could possibly work, > and it's conceptually very easy to understand. And in the implementation I disagree. The implementation magically changes what self references in querymethods. This will break any code that wants to reference self in manager. For example: def get_id_list_by_raw_sql(self, some_params): return cursor.execute("select if from sometable where someparam = %s", (some_param,)) @querymethod def list_interesting_subjects(self, some_params): self.filter(id__in=self.get_id_list_by_raw_sql(some_params)) Now, the above will work just fine if called through manager, but it will not work if called through queryset, because self suddenly refers to the qs, not to the manager. Depending how it is called. So, I would suggest an approach like in my version, where self is always the manager, and you get the queryset by self.get_query_set() (or maybe with self.base_qs, which is a property returning either self.get_query_set() or the queryset called from). The main reason why I think it is a bad idea to "transfer" the method to the qs is that if you do that, you will confuse people (what does self refer to?), forbid some usecases, and gain exactly nothing by doing that. And that can not be changed later on. Why not just let the method be always bound to the manager instance? That is much cleaner. This all assuming I actually understand what the patch does. Can't test it currently... Am I missing something that is gained by currying the method to queryset instead of letting it be bound to the manager? Still another idea is to somehow patch the parameters to the method in querymethod, so that the queryset to chain to would be passed as a parameter to the querymethod. It would either be self.get_query_set() or the queryset called through. - Anssi -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
