-
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
Fixed search strategy for python_logging config #1292
Fixed search strategy for python_logging config #1292
Conversation
94fd2ab
to
14fb677
Compare
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.
Seems reasonable, but can you describe more about how you verified the new and existing behaviour?
'/etc/ros/%s'%(fname), | ||
os.path.join(rosgraph_d, 'conf', fname)]: | ||
for config_dir in [os.path.join(rospkg.get_ros_home(), 'config'), | ||
'/etc/ros/', |
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.
Please use get_etc_ros_dir
, since that accounts for the ROS_ETC_DIR
envvar:
http://docs.ros.org/independent/api/rospkg/html/rospkg_environment.html#get_etc_ros_dir
for config_dir in [os.path.join(rospkg.get_ros_home(), 'config'), | ||
'/etc/ros/', | ||
os.path.join(rosgraph_d, 'conf')]: | ||
for extension in ['.conf', '.yaml', '.yml']: |
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.
Consider os.extsep
, but it isn't used a lot elsewhere in ROS tooling, and .
works on all of the supported ROS platforms.
14fb677
to
bb77dc9
Compare
Updated as suggested. I played a bit and placed files here and there. And from the code it was somehow obvious that when a file is found, the search ends. |
👍 |
I don't think we need to introduce the option for Thank you for the patch. This order makes much more sense. |
The current strategy tries to first find a file named python_logging.conf and, in case none was found, continues searching the yaml version.
A custom configuration in /etc/ros/python_logging.yaml would be ignored as the file python_logging.conf is always present in the share/conf folder of rosgraph.