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

Revision history for this message
Robert Collins (lifeless) wrote :

On Thu, Mar 10, 2011 at 3:16 AM, Gustavo Niemeyer <email address hidden> wrote:
> Review: Needs Fixing
> [1]
>
> 199     +        result = self.store.with_(SQL("tmp as (select 20 as id)")).using(Foo
> 200     +            ).find(Foo.title, SQL("foo.id in (select id from tmp)"));
> 201     +        self.assertEqual(list(result), [u'Title 20'])
>
> We'll have to think a bit more about the interface here.  The proposed one seems
> hard to reason about and will certainly yield confusion regarding the distinction
> between with_() and using(), which are undistinguishable from a developer
> standpoint.

we could rename using to from_ (another keyword :( ...) or perhaps
store.use_with and store.use_from or something like that.

If this patch will break any existing storm features, just let me know
and I'll fix that as a priority. I'm also happy to do modest changes
to make it fit better into storm.

We're going to run a fork in LP for the moment - we need this feature
to solve performance problems.

Cheers,
Rob

« Back to merge proposal