-
Notifications
You must be signed in to change notification settings - Fork 913
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
[rospy] Write log for class method with class name for rospy #877
[rospy] Write log for class method with class name for rospy #877
Conversation
``` $ export ROSCONSOLE_FORMAT='[${severity}] [${time}]: [${node}] [${function}] ${message}' ``` ``` $ python spam.py [INFO] [1472155704.567361]: [/a] [<module>] a [WARN] [1472155704.568193]: [/a] [<module>] a [FATAL] [1472155704.568894]: [/a] [<module>] a [ERROR] [1472155704.569581]: [/a] [<module>] a [INFO] [1472155704.570351]: [/a] [main] a [WARN] [1472155704.571100]: [/a] [main] a [FATAL] [1472155704.571878]: [/a] [main] a [ERROR] [1472155704.572626]: [/a] [main] a [INFO] [1472155704.573439]: [/a] [Test::__init__] a [WARN] [1472155704.574176]: [/a] [Test::__init__] a [FATAL] [1472155704.574937]: [/a] [Test::__init__] a [ERROR] [1472155704.575672]: [/a] [Test::__init__] a ``` ``` $ cat spam.py import rospy rospy.init_node('a') rospy.loginfo('a') rospy.logwarn('a') rospy.logfatal('a') rospy.logerr('a') def main(): rospy.loginfo('a') rospy.logwarn('a') rospy.logfatal('a') rospy.logerr('a') main() class Test(object): def __init__(self): rospy.loginfo('a') rospy.logwarn('a') rospy.logfatal('a') rospy.logerr('a') Test() ```
This patch seems to not resolve the line number. Doesn't this breaks existing behavior when Please also add a test for the new behavior. |
func_name = frame.f_code.co_name | ||
try: | ||
class_name = frame.f_locals['self'].__class__.__name__ | ||
func_name = '%s::%s' % (class_name, func_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
%s::%s
is not the Python way to reference a method of a class. Please use %s.%s
instead.
Fixed. |
pass | ||
return file_name, lineno, func_name | ||
|
||
logging.setLoggerClass(RospyLogger) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should ensure not to overwrite custom logger classes already being set by user code. See https://docs.python.org/2/library/logging.html#logging.getLoggerClass on how to achieve this.
Could you please also cover that case in the test.
Fixed. |
Please use the approach described in the link from #877 (comment) since it also allows to customize the logger if a user defined class has been set before. |
But below code overwrites the custom logger class. Is that ok? class MyLogger(logging.getLoggerClass()):
# ... override behaviour here |
Yes, but it creates the subclass not of the original logger but of the currently set logger class which could be set by a user before. So it will maintain all other changes from the custom class (except the overridden functionality). |
484af79
to
901290a
Compare
901290a
to
76835b0
Compare
Ok, fixed. |
The patch currently fails one of the new tests. |
Fixed. |
@@ -0,0 +1,133 @@ | |||
# Software License Agreement (BSD License) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this file in a subfolder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file shouldn't repeat all the tests from the other file but only check the UserCustomLogger
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file shouldn't repeat all the tests from the other file but only check the UserCustomLogger.
Ok I will fix it.
This reverts commit 6ac0137.
Well, .. it still fails
|
Addressed by #1043. |
For #875