[Catalyst-dev] Patch: Make -Log Work

[Matt S Trout]

On Wed, Aug 06, 2008 at 09:16:38AM -0700, David E. Wheeler wrote:
> On Aug 5, 2008, at 17:26, Tomas Doran wrote:
> 
> >Looks good to me - couple of minor comments:
> >
> >Would the test in the patch be better being part of t/ 
> >unit_core_log.t as there are already logging tests there, so that's  
> >where I'd logically look for them. I agree where you've put them  
> >would be the right place if there were unit tests for all the setup  
> >code - but as there aren't, I think the current location is a bit  
> >confusing..
> 
> Well, I'm not testing the log, I'm testing setup. So I added it so  
> that now there *is* a place to test setup, and in the hope that others  
> might put more setup tests there. Honestly, I was astonished to find  
> no tests for setup.
> 
> >You split on \s*,\s* (to remove whitespace by commas), but you don't  
> >trim whitespace at the start or end of the string, so I can break  
> >things by saying '-Log=  error,fatal  '
> 
> Heh, fair enough. Added.
> 
> >Applying your patch seems to give me a couple of lines which read:
> >
> >   if (defined ) {
> >   } my $env_debug = Catalyst::Utils::env_value( $class, 'DEBUG' );
> >
> >I guess that the if (defined ) {} construct isn't meant to be there  
> >as it's a no-op?
> 
> Yeah, pasto. Fixed.
> 
> >Also, whilst you're there - why not expand the pod on setup_log to  
> >be slightly more than a stub?
> 
> Okay.
> 
> >>Also, if someone specifies -Debug, shouldn't it enable *all* log  
> >>levels, not just debug?
> >
> >Yes, however I believed that it already did - you certainly see all  
> >the debug levels when you start an app in the development server  
> >with -Debug
> 
> Yes, but check out the test when -Debug is specified:
> 
> ok !$log->is_warn,  'Warnings should be disabled';
> ok !$log->is_error, 'Errors should be disabled';
> ok !$log->is_fatal, 'Fatal errors should be disabled';
> ok !$log->is_info,  'Info should be disabled';
> ok $log->is_debug,  'Debugging should be enabled';

That's odd. I think the reasoning is that debug should only turn on
$c->debug and ensure the $c->log->debug calls go somewhere?

-- 
      Matt S Trout       Need help with your Catalyst or DBIx::Class project?
   Technical Director                    http://www.shadowcat.co.uk/catalyst/
 Shadowcat Systems Ltd.  Want a managed development or deployment platform?
http://chainsawblues.vox.com/            http://www.shadowcat.co.uk/servers/