On Sat, 10 May 2008 20:36:14 +0800 "Russell Keith-Magee" <[EMAIL PROTECTED]> wrote: > The basic idea that you are proposing has been made before - for > example, several of the discussions about adding aggregations to the > ORM have included a capability similar to the one you propose. > However, this is the first time to my knowledge that code has been > offered.
I know the aggregation stuff and used it as example when I wrote this
code. As you can see I provide also a base class with an as_sql method
which is overridden in derived classes. And I adopted the way the qn
function is passed.
> * I don't see why LiteralExpr is required. Surely a literal should
> continue to work as is; if a literal is involved in an expression with
> a ColumnExpr(), the definition of ColumnExpr should be smart enough to
> merge the literal into the resultant expression.
You can still do model.objects.update(foo=42) with my patch, because
of 42 is casted to a LiteralExpr under the hood. I could even make it
possible to do model.objects.update(foo=CurrentExpr() + 42). But there
is no way to enable model.objects.update(foo=42 + CurrentExpr()),
because of in this case int.__add__ instead of Expression.__add__ is
called, so I decided to introduce LiteralExpr. But I agree that it
would be cool, if you could use a literal as it is.
> * foo=CurrentExpr() is really just the same as foo=ColumnExpr('foo'),
> isn't it? Why include two ways of providing the reference?
Following statements are indeed equivalent:
model.objects.update(foo=CurrentExpr() + LiteralExpr(1))
model.objects.update(foo=ColumnExpr('foo') + LiteralExpr(1))
But first is less code, because of you don't have to specify the column
and I think second does violate the DRY slightly. But the really
interesting thing is that you can do following:
increment_expr = CurrentExpr() + LiteralExpr(1)
model_1.objects.update(foo=increment_expr)
model_1.objects.update(bar=increment_expr)
model_2.objects.update(foo=increment_expr)
There is actually no need for CurrentExpr, it is just there because of
DRY, but I already thought about using ColumnExpr() (a ColumnExpr
instance without a given column) instead of CurrentExpr().
> * ColumnExpr is a bit cumbersome for something that is really exposing
> the primitives of the underlying SQL. Given that LiteralExpr and
> CurrentExpr aren't really required, you only need one object for
> expressions - the ability to reference a field. I would suggest
> adopting the syntax from the earlier ORM aggregation discussions: F()
> (for 'Field'). This follow the lead of Q() to describe Query objects.
I also don't like the name *Expr. I already considered to use something
similar to Q(), but I think this is to confusing as long we have
multiple Expression objects. If we will decide to drop LiteralExpr and
CurrentExpr, I would like to use F(), as you proposed instead of.
> * The examples you give are driven by use cases in the update()
> clause. This is certainly a useful application of the idea, but it
> would be good to ensure that the same syntax can be used elsewhere -
> most notably, in filter() and exclude() clauses.
I think this is a good idea. The only reason why I only supporting
update() at the moment is, why I need this feature for a modified
pre-ordered tree traversal, app I am currently working on. And if you
know mptt, you know that you have to increment or decrement
values ordinary over the half table, when you change anything, which
really sucks if you can not do this with a single UPDATE statement. ;)
> On top of these criticisms, there is at least one very good reason
> that this patch won't get merged as-is - it doesn't contain any
> documentation or test cases. What you are proposing is a fairly big
> feature addition - documentation and tests would be mandatory before
> the patch was accepted. I can understand why you might have left off
> documentation until your idea was accepted, but I (and the other core
> developers) are rarely convinced that code works unless there is a
> comprehensive test suite to back it up.
I know this. But there is no sense do this at the moment. As you
already noted there are still a few points to discuss and probably
things to change on this code. So I will write documentation and tests
when this is finished, because of otherwise I have to do this work
twice.
Regards
Sebastian Noack
signature.asc
Description: PGP signature
