Login | Register For Free | Help
Search for: (Advanced)

Mailing List Archive: Catalyst: Dev

Patch: Make -Log Work

 

 

Catalyst dev RSS feed   Index | Next | Previous | View Threaded


david at kineticode

Aug 5, 2008, 1:19 PM

Post #1 of 7 (2929 views)
Permalink
Patch: Make -Log Work

Howdy,

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

Also, if someone specifies -Debug, shouldn't it enable *all* log
levels, not just debug?

Best,

David
Attachments: catalyst_log_setup.patch (2.65 KB)


bobtfish at bobtfish

Aug 5, 2008, 5:26 PM

Post #2 of 7 (2780 views)
Permalink
Re: Patch: Make -Log Work [In reply to]

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 [at] lists
http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst-dev


david at kineticode

Aug 6, 2008, 9:16 AM

Post #3 of 7 (2776 views)
Permalink
Re: Patch: Make -Log Work [In reply to]

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
Attachments: catalyst_log_setup2.patch (1.69 KB)


dbix-class at trout

Aug 7, 2008, 6:48 PM

Post #4 of 7 (2773 views)
Permalink
Re: Patch: Make -Log Work [In reply to]

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 [at] lists
http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst-dev


david at kineticode

Aug 7, 2008, 8:56 PM

Post #5 of 7 (2774 views)
Permalink
Re: Patch: Make -Log Work [In reply to]

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 [at] lists
http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst-dev


david at kineticode

Sep 3, 2008, 3:13 PM

Post #6 of 7 (2643 views)
Permalink
Re: Patch: Make -Log Work [In reply to]

*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 [at] lists
> http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst-dev


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


bobtfish at bobtfish

Dec 10, 2008, 2:00 AM

Post #7 of 7 (2244 views)
Permalink
Re: Patch: Make -Log Work [In reply to]

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 [at] lists
http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst-dev

Catalyst dev RSS feed   Index | Next | Previous | View Threaded
 
 


Interested in having your list archived? Contact Gossamer Threads
 
  Web Applications & Managed Hosting Powered by Gossamer Threads Inc.