Code review comment for lp:~lifeless/storm/with

Revision history for this message
James Henstridge (jamesh) wrote :

I guess I was misreading the syntax of the with_query, so column names are not required. For reference, here is the syntax:

  with_query_name [ ( column_name [, ...] ) ] AS ( select | insert | update | delete )

While the column list isn't required, it seems more robust to list the column names for cases where you would have duplicate names otherwise. I also don't think we'd make life any easier by automatically aliasing the columns to something like _1, _2, etc.

As far as the actual syntax goes, I'd think it common for users to use different variations on the query on each use (Robert can probably verify whether this is correct), so your second suggestion of tying the query to a class defined using standard class syntax seems a bit verbose. Something where you could link the query to an existing template class would seem more natural.

One other question Robert might have some input on: is it actually beneficial to use a full table class to represent the "with" table? They may not have a well defined primary key, and the table will be gone once the query completes, so it isn't clear you'd want to clutter the object cache with rows from the table. And are there many cases where you'd want to return objects from a "with" table from a Store.find() call, or are they just for use in the WHERE clause?

« Back to merge proposal