first pass docs and code #41

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
1 participant
@jjn1056
Member

jjn1056 commented Jun 27, 2014

do not merge, for discussion only (JNAP)

@jjn1056

This comment has been minimized.

Show comment
Hide comment
@jjn1056

jjn1056 Jun 28, 2014

Member

A few notes:

in changing the debug notes to use trace level 1, the vast majority looked like this and were straightforward to port:

$c->log->debug(...) if $c->debug

However there were a handful that used log methods higher than 'debug', like warn or error. (2 or 3 cases). I think it would be ok for us to port those as well. There seems to have been some sort of attempt to raise a few notices to a higher level (would be a level 2 or 3 under the new proposed system) but I made them all level one since I think this is going to be what people expect.

There are quite a few places that did if($c->debug) { .... } mostly around building up large amounts of debug info that I ported to if $c->trace_level which seems to be fine (maybe we should have a more explicit $c->has_log_level

in a few instances Catalyst is actually doing $c->log->$method outside of debug mode. For example there is a warning around cookies not be properly found... Not sure if those should have been moved to trace or if we should have something else. My feeling now is that catalyst could raise some sort of unusual or exception conditions rather than just sent an alert to debug that nobody cares about...

Member

jjn1056 commented Jun 28, 2014

A few notes:

in changing the debug notes to use trace level 1, the vast majority looked like this and were straightforward to port:

$c->log->debug(...) if $c->debug

However there were a handful that used log methods higher than 'debug', like warn or error. (2 or 3 cases). I think it would be ok for us to port those as well. There seems to have been some sort of attempt to raise a few notices to a higher level (would be a level 2 or 3 under the new proposed system) but I made them all level one since I think this is going to be what people expect.

There are quite a few places that did if($c->debug) { .... } mostly around building up large amounts of debug info that I ported to if $c->trace_level which seems to be fine (maybe we should have a more explicit $c->has_log_level

in a few instances Catalyst is actually doing $c->log->$method outside of debug mode. For example there is a warning around cookies not be properly found... Not sure if those should have been moved to trace or if we should have something else. My feeling now is that catalyst could raise some sort of unusual or exception conditions rather than just sent an alert to debug that nobody cares about...

@jjn1056

This comment has been minimized.

Show comment
Hide comment
@jjn1056

jjn1056 Jun 28, 2014

Member

not to scope creep here but perhaps I have an excuse to start slipping return multilevel, or https://metacpan.org/pod/Worlogog::Incident in to handle the "Catalyst has an unexpected situation".

I'd like to cleanly separate internal application tracing from actually 'Houston, we have a problem" type situations in Catalyst core code. Feels to me like this is something the user of the framework would like to have influence over. We could say Catalyst raises certain conditions and define default behaviors for them ($app->log->error is probably fine and would be good for backcompat). Then a user could override how that is handled.

Member

jjn1056 commented Jun 28, 2014

not to scope creep here but perhaps I have an excuse to start slipping return multilevel, or https://metacpan.org/pod/Worlogog::Incident in to handle the "Catalyst has an unexpected situation".

I'd like to cleanly separate internal application tracing from actually 'Houston, we have a problem" type situations in Catalyst core code. Feels to me like this is something the user of the framework would like to have influence over. We could say Catalyst raises certain conditions and define default behaviors for them ($app->log->error is probably fine and would be good for backcompat). Then a user could override how that is handled.

@jjn1056

This comment has been minimized.

Show comment
Hide comment
@jjn1056

jjn1056 Jun 28, 2014

Member

perhaps a not too terrible thing here is to throw a type of Catalyst execption object that handle those by default. Then someone could check the $c->error stack and handle it special.

Member

jjn1056 commented Jun 28, 2014

perhaps a not too terrible thing here is to throw a type of Catalyst execption object that handle those by default. Then someone could check the $c->error stack and handle it special.

@jjn1056

This comment has been minimized.

Show comment
Hide comment
@jjn1056

jjn1056 Jun 28, 2014

Member

the more I look at this I think we need a has_trace_level for backcompat

sub has_trace_level {
my $app = shift;
return defined($app->trace_level) ? $app->trace_level : $a->debug;
}

This would do a slightly better job of handling edge cases around when we used to call debug, just in cause setup_trace hasn't yet been run (the whole setup_* chain is a pile of mess at this point).

Member

jjn1056 commented Jun 28, 2014

the more I look at this I think we need a has_trace_level for backcompat

sub has_trace_level {
my $app = shift;
return defined($app->trace_level) ? $app->trace_level : $a->debug;
}

This would do a slightly better job of handling edge cases around when we used to call debug, just in cause setup_trace hasn't yet been run (the whole setup_* chain is a pile of mess at this point).

@jjn1056

This comment has been minimized.

Show comment
Hide comment
@jjn1056

jjn1056 Jun 30, 2014

Member

Well, after sleeping on it a reviewing the source I feel pretty good. I'm proposing this patch for inclusion. The only unanswered question I really have is what to do with the fact that debug mode also controls if you get the developer error screen or the 'production' default error. I think when we move that shit out to the middleware layer we'll have better control. I'd like to if possible decouple any sort of debug or trace mode from what error messages you get.

Member

jjn1056 commented Jun 30, 2014

Well, after sleeping on it a reviewing the source I feel pretty good. I'm proposing this patch for inclusion. The only unanswered question I really have is what to do with the fact that debug mode also controls if you get the developer error screen or the 'production' default error. I think when we move that shit out to the middleware layer we'll have better control. I'd like to if possible decouple any sort of debug or trace mode from what error messages you get.

@jjn1056 jjn1056 closed this Jul 14, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment