Re: [log4perl-devel] New Levels

[Erik W. S. <er...@se...>]

Kevin Goess wrote:

> Nice work, a couple of questions:
>
> 1) Why are you reversing the order of the PRIORITY int's from log4j 
> "these are actually REVERSED from log4j'cuz it makes our life 
> easier."?  How so, it's an arbitrary internal ordering, isn't it?  
> While I understand that if it's arbitrary and internal then it doesn't 
> matter, I think we should do our part to fight entropy by keeping 
> unnecessary divergence to a minimun. 

So, in reality, the only place those constants (FOO_INT) are used is to 
populate the add_prioritiy() calls a bit later... they aren't exported 
at all. So only their name comes from log4j. What we could do is set 
those constants to be what log4j has and then have some other hash table 
that does the conversion from a descending order to an acending one.

>
> 2) I agree that my suggested BEGIN block is not the way to do it, but 
> I'm worried about create_custom_level().  If they call it after the 
> engine has seen init(), it could result in undefined behavior.  Could 
> we  just disallow it if $INITIALIZED is already set?

Yeah, that make sense to me.

>
> 3) create_custom_level() should really be in Level.pm not Logger.pm, 
> don't you think?  And the interface to calling it could actually be in 
> Log4perl.pm, a pass-thru.

Hmmm... yeah, I'd buy that. I'd also buy create_log_level_methods moving 
there too.

>
> 4) For my own education, can you explain the rationale behind "This is 
> a bit better way to create code on the fly than eval'ing strings."  I 
> understand that it may be faster, but in this case I can see 
> advantages to the string eval in that you don't have to turn off 
> strict refs, it catches cases where the user tries to name a level 
> like WARN-LITE (where "warn-lite" is not a valid perl subroutine name, 
> otherwise the error isn't caught until the user tries to call 
> $log->warn-lite(..)), and in this case I think the intent of the code 
> reads more clearly as an eval'd string.  So my own knowledge is 
> deficient--I can't see where the improvement is.

There are a couple, although your point above is well-taken. The primary 
(pragmatic) one is that I was having a devil of a time implementing a 
function as the (main package) variable $WARNLIGHT (or whatever I tried 
to use) is not imported in the eval. I'm still not clear on the cause, 
but the chat whores in #perl suggested that there was a known issue with 
variable scoping and eval'ing to create subroutines, and I was better 
off to use the code template / closure method (which is what I did).

The other two reasons which if I didn't hit the problem above I'd have 
ignored are:
(a) eval'ing is slower compared with creating the code refs
(b) the code refs are created at Perl compile time, which means if I go 
in and hack the code and put in a typo, Perl catches it. If it's a 
string and I fat-finger sometihng, you don't see that until the eval.

To your point, perhaps a check beforehand to make sure that $lclevel 
conforms to Perl function name conventions and dies with a descriptive 
error?

> 5) my $cust_prio = int(($priority{$after} + $next_prio) / 2);
> Just to be complete, we should check that they haven't added so many 
> priorities that we've run out of halves and are stepping on existing 
> numbers, shouldn't we?

Oh, sure. Although whomever makes the 10,000 levels between two we give 
'em deserves what he or she gets, IMHO. :)

Did one of you guys check in the patch? I'll make the above changes, 
just wanted to know from which repository to work from. :)

-e

>
> Erik W. Selberg wrote:
>
>> Actually, ignore my previous patch and check this one out. I took the 
>> liberty of re-doing how the code is generated using the following syntax:
>>
>> *{ <foo> } = sub { <bar> }
>>
>> vs:
>>
>> $code = "sub foo { <bar> }"; eval $code;
>>
>> which enabled me to re-use it to auto-create new levels on the fly 
>> using:
>>
>> Log::Log4perl::Logger::create_custom_level("LITEWARN", "WARN");
>>
>> I also re-did 025CustLevels.t with this in mind, as I think it's 
>> nicer than the BEGIN block method.
>>
>> While I was at it, I also updated logdie and friends so they die 
>> regardless of the OFF setting (and changed appropriate test classes), 
>> and implemented the newer constants that Mike has (although note I'm 
>> pretty sure this patch will clobber his).
>>
>> Cheers,
>> -e
>>
>>
>>
>> -------------------------------------------------------
>> This sf.net email is sponsored by: OSDN - Tired of that same old
>> cell phone?  Get a new here for FREE!
>> https://www.inphonic.com/r.asp?r=sourceforge1&refcode1=vs3390
>> _______________________________________________
>> log4perl-devel mailing list
>> log...@li...
>> https://lists.sourceforge.net/lists/listinfo/log4perl-devel
>
>
>