[Catalyst-dev] Patch: Make -Log Work

[David E. Wheeler]

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';

Only is_debug returns true.

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

Yeah, I've also seen other levels output when dbugging is enabled, but  
the Catalyst::Log object doesn't seem to realize it.

Anyway, thanks for the feedback. New patch attached.

Best,

David
-------------- next part --------------
A non-text attachment was scrubbed...
Name: catalyst_log_setup2.patch
Type: application/octet-stream
Size: 1732 bytes
Desc: not available
Url : http://lists.scsys.co.uk/pipermail/catalyst-dev/attachments/20080806/d0cc0219/catalyst_log_setup2.obj