[Dbix-class] '+select'/'+as' is broken when chaining resultsets

[BUCHMULLER Norbert]

On Mon, 27 Oct 2008 22:41:26 +0100 Peter Rabbitson
<rabbit+list at rabbit.us> wrote:

> Indeed it was. The functionality patch looks good in itself. However I
> am not sure that the tests changes are appropriate. The test name is
> 88result_set_column which implies calling methods on a resultset. Yet
> you changed a number of $rs->accessor invocations to
> $rs->first->accessor. Please explain why you did this.

When I wrote the test, I found another bug in the test
case: $psrs->get_column() returns a DBIx::Class::ResultSetColumn object
even if no column exists with that name, so all the '+select/+as' tests
are kind of useless (as they unconditionally pass, even if no
such column exists in the resultset).. :-( I added a test for that bug,
too ('+select/+as nonexistent_column').

So, the patch for t/88_result_set_column.t contains modifications related
to 3 different issues:

1. a new test for chained search() and '+select'/'+as'
2. a new test for a $rs->get_column() for a nonexistent column
3. $rs->get_column() calls replaced with $rs->first->get_column() calls

Regarding 1.: I think that should be OK.

Regarding 2.: we should have such a test IMHO, otherwise we cannot trust
the other test cases (as we cannot be sure that a column really exists -
which the state is currently).

Regarding 3.: maybe I misunderstood something, in that case pls give me
some pointer how to solve it better. (First I looked into
DBIx::Class::ResultSetColumn, but found that in C<new> I should not check
if the column exists in the resultset as C<new> should not touch the
database - as far as I see the design. Then I looked into
DBIx::Class::ResultSet, but found that C<get_column> does not look like
the ideal place for such a change, too. But here I'm possibly wrong.)

Would it help if I send the changes broken up into 3 (stacked) patches?
(So that you can review the changes separately.)

norbi