Mailing List Archive

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


_______________________________________________
Catalyst-dev mailing list
Catalyst-dev@lists.scsys.co.uk
http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst-dev

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

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/

_______________________________________________
Catalyst-dev mailing list
Catalyst-dev@lists.scsys.co.uk
http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst-dev

On Aug 7, 2008, at 18:48, Matt S Trout wrote:

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

In every other system I've used, log levels were additive. So if I set
it to "warn", I'd get warnings, errors, and fatals. By that logic, if
you set it to debug, you should get everything. But Catalyst::Log
doesn't work that way. Each level is discreet. So I think that whoever
hacked this in was assuming that "debug" would enable everything, and
that's a pretty reasonable assumption, IMHO.

I can easily modify the patch to enable all the log levels. But might
it not be reasonable to also make levels set by the constructor (as
opposed to setting them by enable or disable) additive?

Best,

David

_______________________________________________
Catalyst-dev mailing list
Catalyst-dev@lists.scsys.co.uk
http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst-dev

*bump*

http://lists.scsys.co.uk/pipermail/catalyst-dev/2008-August/subject.html#start

Best,

David

On Aug 7, 2008, at 20:56, David E. Wheeler wrote:

> On Aug 7, 2008, at 18:48, Matt S Trout wrote:
>
>>> 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?
>
> In every other system I've used, log levels were additive. So if I
> set it to "warn", I'd get warnings, errors, and fatals. By that
> logic, if you set it to debug, you should get everything. But
> Catalyst::Log doesn't work that way. Each level is discreet. So I
> think that whoever hacked this in was assuming that "debug" would
> enable everything, and that's a pretty reasonable assumption, IMHO.
>
> I can easily modify the patch to enable all the log levels. But
> might it not be reasonable to also make levels set by the
> constructor (as opposed to setting them by enable or disable)
> additive?
>
> Best,
>
> David
>
> _______________________________________________
> Catalyst-dev mailing list
> Catalyst-dev@lists.scsys.co.uk
> http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst-dev


_______________________________________________
Catalyst-dev mailing list
Catalyst-dev@lists.scsys.co.uk
http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst-dev

On 3 Sep 2008, at 23:13, David E. Wheeler wrote:

> *bump*
>
> http://lists.scsys.co.uk/pipermail/catalyst-dev/2008-August/
> subject.html#start
>

Having dropped this on the floor for 3 months, I managed to find it
again tonight.

I've sewn your two patches together and applied them r8809.

http://dev.catalyst.perl.org/svnweb/Catalyst/revision?rev=8809

Apologies for the delay.

Cheers
t0m


_______________________________________________
Catalyst-dev mailing list
Catalyst-dev@lists.scsys.co.uk
http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst-dev