Re: Some questions on CDBI code

[prev] [thread] [next] [Date index for 2005/07/05]

tony-cdbitalk@xxxxx.xxx wrote:

>On Tue, Jul 05, 2005 at 01:25:53PM +1000, leif.eriksen@xxx.xxx.xx wrote:
>  
>
>Thanks for doing this! The test coverage is quite good at the minute (as
>long as you can run it against multiple databases), but there's always
>room for improvement!
>
>  
>
Pleasure is all mine - btw, I've really only got all the closures and 
"return sub { ... }" paths left - and there the hardest, because they 
have to be done in two parts - one is setting them up at 'construction' 
time and the other is feeding them data at 'runtime' or 'invocation-time'

>confess() is probably too severe. We probably want $self->_croak, which
>could be overridden to just warn, so the fall through would be OK.
>
>  
>
OK, that'll be part of the patch ...

>>In Class::DBI::Iterator::new(), there is no attempt to check the 
>>    
>>
>I'm not sure how we can check that we have a 'sensible' package name, as
>'main' is an entirely valid package. It may not make a lot of sense to
>give it a 'main', but I'd prefer to keep this duck-typed. As long as the
>class we're given can merrily do the 'construct()' etc, I don't see why
>we should impose restrictions on it.
>
>  
>
OK, I've leave the $me alone, would you rather I did a 
$them->can('construct') test ? That is more constrained than a 
$them->isa('Class::DBI'). I know a class could support this via an 
AUTOLOADER, and hence can() will fail, but I guess the aim is to check 
that the supplied class supports the required interface, rather than 
force a particular inheritance.
Code would be

sub new {
	my ($me, $them, $data, @mapper) = @_;
	
	return unless $them->can('construct'); # supports required i/f

	bless {
		_class  => $them,
		_data   => $data,
		_mapper => [@mapper],
		_place  => 0,
		},
		ref $me || $me;
}


>>Lastly, I've spent a lot of time going cross-eyed at 
>>Class::DBI::stringify_self().
>>
>This is one of the issues I came across when I was playing with
>Devel::Cover. To get full coverage a lot of the time you need to call
>methods as functions and make sure the code handles this. I've always
>been undecided as to what to do here. In the general case I' rather add
>unlikely code like "Class::DBI::stringify_self(0)" code to the tests to
>get coverage than adding contorted checks to the code. In this specific
>case I have no strong opinions.
>
>  
>
OK, though this is more removing unreachable code than adding a check to 
the code (removing code, rather than adding code). I will bear in mind 
the advice about adding checks in code vs unlikely code in tests.

Thanks again, hopefully I'll be finished soon (I'm racing to see if I 
can get most of this added before 1.00 - a constraint for me rather than 
for you).

        -- 
        Leif Eriksen
Snr Developer
http://www.hpa.com.au/
phone: +61 3 9217 5545
email: leif.eriksen@xxx.xxx.xx