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