Code review comment for lp://staging/~cr3/storm/reference_is_in

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

> Interestingly, I had originally implemented the is_in method on references
> using the Or expression but I later found it preferable to use the In
> expression. However, in order to support a wider range of backends, I'm fine
> with reverting to the Or expression.
>
> Thanks for suggesting to test the corner case when passing an empty list. I
> made sure to write the corresponding test which then failed when just uring
> the Or expression. So, when the given argument to the is_in method is an empty
> list, it simply returns False which seems to work fine.
>
> The branch has been updated with these changes.

With your new changes, passing an Expr object to is_in() will now fail. With the previous version of your patch it was possible to do something like:

    Employee.company.is_in(Select(Company.id, Company.name.like("%corp%")))

The cast to list() in your new version would fail here.

« Back to merge proposal