ResultSet doesn't implement __nonzero__

[James Westby]

SQLObjectResultSet implements __nonzero__ but the native Storm ResultSet does not. This makes it harder to migrate old code.

[James Westby (james-w)]

btw, this is now referred to from an XXX in my branch.

Thanks,

James

[Robert Collins (lifeless)]

I think we should get rid of the sqlobject idioms, as we're building
on storm, not sqlobject.

[James Westby (james-w)]

I don't have a strong opinion which is used, but it should be consistent across all queries
to avoid nasties like this.

Thanks,

James

[Julian Edwards (julian-edwards)]

On Sunday 01 August 2010 21:11:24 you wrote:
> Public bug reported:
>
> Hi,
>
> getPackagesetsForUploader uses Store.find, which doesn't return an
> SQLObject result set.
>
> This breaks assumptions, such as __nonzero__ being implemented, which
> caused a (small) bug in checkUpload().

There's probably many many places like this throughout the LP code base where
we're mixing SQLObject and Storm. :/

A good start would be to implement __nonzero__ for ResultSet. And then
converting our SQLObject classes to Storm can be done over time.

BTW James what was the bug in checkUpload() ?

[Julian Edwards (julian-edwards)]

On Sunday 01 August 2010 21:30:36 you wrote:
> I think we should get rid of the sqlobject idioms, as we're building
> on storm, not sqlobject.

I agree, and I don't think this is a bug. The real bug is in the code that's
using the result set.

[Robert Collins (lifeless)]

On Mon, Aug 2, 2010 at 8:46 AM, James Westby <email address hidden> wrote:
> I don't have a strong opinion which is used, but it should be consistent across all queries
> to avoid nasties like this.

I think that making a good transition is important, but we shouldn't
block on having it totally perfect. OTOH we should also provide a
ratchet mechanism and finish such transitions, which is what appears
to be incomplete here.

Would you like to file a bug on Storm - __nonzero__ seems sensible to
have on a result set anyhow..

[Robert Collins (lifeless)]

two things:

firstly, if you want to move something off of soyuz, you can click the
drop-down and just type in a new product.

Secondly, what do we use instead of __nonzero__ in new, storm-aware code today?

[Jamu Kakar (jkakar)]

Instead of using __nonzero__ we recommend using
ResultSet.is_empty(). I do wonder if it's worth adding
__nonzero__... on the one hand it feels like it could be nice
(obviously people are asking for it), on the other hand it feels
magical and that it would be better to just use is_empty.

Is there a case where is_empty is not sufficient?

[Julian Edwards (julian-edwards)]

On Monday 02 August 2010 09:38:37 Jamu Kakar wrote:
> Instead of using __nonzero__ we recommend using
> ResultSet.is_empty(). I do wonder if it's worth adding
> __nonzero__... on the one hand it feels like it could be nice
> (obviously people are asking for it), on the other hand it feels
> magical and that it would be better to just use is_empty.
>
> Is there a case where is_empty is not sufficient?

I think in theory it's fine to use is_empty (I hate magical stuff too), when
we migrate code from SQLObject it should use that instead although for
convenience __nonzero__ would be nice to have.

I had a look at the code for is_empty and it seems it's got the same problem
that you fixed recently for SQLObjectResultSet.__nonzero__ where it's not
removing the result ordering before issuing the query. I think .any() should
be doing that as well?

[Julian Edwards (julian-edwards)]

On Monday 02 August 2010 09:19:31 Robert Collins wrote:
> two things:
>
> firstly, if you want to move something off of soyuz, you can click the
> drop-down and just type in a new product.

I know, I specifically did not want to do that though, so Soyuz subscribers
still get bug mail about this.

[Jamu Kakar (jkakar)]

Yeah, I'm aware of the issue with ResultSet.any not stripping ORDER
BY. I have a branch in progress to fix it, but there's some test
fallout I need to take care of before I can land it.

[James Westby (james-w)]

On Mon, 02 Aug 2010 08:38:37 -0000, Jamu Kakar <email address hidden> wrote:
> Instead of using __nonzero__ we recommend using
> ResultSet.is_empty(). I do wonder if it's worth adding
> __nonzero__... on the one hand it feels like it could be nice
> (obviously people are asking for it), on the other hand it feels
> magical and that it would be better to just use is_empty.
>
> Is there a case where is_empty is not sufficient?

It would be good to have it on SQLObjectResultSet too, so that we can
start migrating code to the explicit check, and they can be robust
against changes in the result set that you get.

Currently if you want to support both you have to use .count(), which
would be bad.

Thanks,

James

[Julian Edwards (julian-edwards)]

Sounds like this bug should be titled "SQLObjectResultSet doesn't implement is_empty" then :)