david at kineticode
Aug 6, 2008, 9:16 AM
Post #3 of 7
(2295 views)
Permalink
|
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
|
catalyst_log_setup.patch
