[Catalyst-dev] Patch: Make -Log Work

[Tomas Doran]

On 5 Aug 2008, at 21:19, David E. Wheeler wrote:

> I noticed that the -Log parameter to setup wasn't working properly.  
> I wrote a test for it and then fixed it. Patch attached.
>

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..

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  '

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?

Also, whilst you're there - why not expand the pod on setup_log to be  
slightly more than a stub?

> 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

However I'm not very (at all) familiar with Catalyst's logging code,  
it's late and I've haven't gone and read the source - so I may be  
(and probably am) wrong.

Cheers
t0m