Nice work, I think that's a lot better. The only thing that now worries me
is that order_by doesn't accept sql parameters for some reason. I'm not
sure how many expressions produce sql_params and if they'd be useful in an
ordering context. It might be worth looking into that a little more.
I'm not sure when this branch will get merged, but I'm fairly confident it
will be this time around. Anssi did another review pass and contributed
some nice fixes. He indicated that he'd try to do another pass next week
(this week?), but that's all very dependent on the available time he has.
I'm hoping the merge will be soon, as it gets quite annoying fixing the
merge conflicts every time I rebase onto master :P
As for function definitions, I think that should be part of a separate PR.
We don't want the implementations of various functions to hold up the
merging of expressions or ordering expressions. There's also the small
design question of where these functions should live. I was thinking of
django.db.models.functions or django.db.models.expressions.functions, but
I'm not married to that idea. We also need to determine a set of functions
that we'd like to commit to supporting in core. Further, if there are
differences in implementations per backend, should the differences be
surfaced in each of the backends, or should they reside entirely within the
expression using as_oracle and as_mysql? I would prefer the as_{vendor}
approach, as it allows other backends to easily implement without worrying
about connection.backend.yet_another_backend_method_to_support, but that'd
be a fairly big deviation from the way things are typically done with
supported backends. Perhaps we can start another discussion on the ML so we
can address these concerns without it being hidden inside this thread?
Josh
On Monday, 16 June 2014 03:45:02 UTC+10, Tim Martin wrote:
>
> On Wednesday, 11 June 2014 12:28:20 UTC+1, Josh Smeaton wrote:
>>
>> > Model.objects.filter(...).order_by( F('fld_a').desc(),
>> F('fld_b')).asc() )
>> > Model.objects.filter(...).order_by( (F('fld_a')+F('fld_b')).desc() )
>>
>> I actually really like this. It's simple, clear, and gets around the
>> issues being discussed. It still requires Expressions to know about
>> ordering (to implement the asc/desc methods) but I think that's fine.
>>
>> The other problem with having ordering objects being of a different type
>> means that you would have to call asc or desc. But now I think of it, the
>> order_by method could just call asc, by default, on any expressions passed
>> in.
>>
>
> I agree, this looks like a good API and fixes a lot of my concerns. I've
> got this implemented in my branch now (
> https://github.com/timmartin/django/tree/order_by_expressions)
>
> Other than some small bits of tidying, I think the only thing missing here
> is documentation and a bunch of instances of expressions.Func() for the SQL
> functions we want to support.
>
> Josh, what's the likely timeline of getting your branch merged in? And
> will your change include any function definitions, or shall I do that as
> part of my work?
>
> Tim
>
--
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 [email protected].
To post to this group, send email to [email protected].
Visit this group at http://groups.google.com/group/django-developers.
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-developers/48780269-efe1-44ae-89a5-a5601f49c6e4%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
