Skip to content

carp warn user from perspective of caller and already ignore Log::Log4perl::Appender-Class #107

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Sadrak
Copy link

@Sadrak Sadrak commented Mar 5, 2021

The carp()-Message from Log::Log4perl::Appender jump over the important package who call the log-method and report from the next package. Simple remove the increase for $Carp::CarpLevel fix the problem.

From the Carp pod for carp():

# warn user (from perspective of caller)
carp "string trimmed to 80 chars";

Short example:

$ cat <<EOF > test.pl 
package Test;
use Log::Log4perl qw(:easy);
Log::Log4perl->easy_init($ERROR);
sub test { ERROR undef; }
package main;
Test::test();
EOF
$ perl test.pl 
Warning: Log message argument #1 undefined at test.pl line 6.
Use of uninitialized value in join or string at /usr/local/lib/perl5/site_perl/5.32.1/Log/Log4perl/Appender.pm line 189.
2021/03/05 07:33:45

This should say the error occur in line 4 not line 6.

@mohawk2
Copy link
Collaborator

mohawk2 commented Jun 1, 2022

This would be a break in the API; $caller_depth is documented in the FAQ in several places. From an application's point of view, it's going to be rare that a logger-calling function is directly responsible for the undef being passed into Log4perl, instead it will normally be due to the function's caller (e.g. in your example, it might be because it should have passed an argument and didn't).

I'm not totally opposed to this idea, but you need to make a stronger case that such a change would be worth the harm it would cause.

@mohawk2 mohawk2 force-pushed the master branch 4 times, most recently from 01d6239 to 4c69f91 Compare June 1, 2022 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants