Skip to content
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

Simplified internal loggers #1290

Merged

Conversation

mgrrx
Copy link
Contributor

@mgrrx mgrrx commented Jan 1, 2018

This allows using args and kwargs as in the python logging module

@@ -131,18 +131,10 @@ def parse_rosrpc_uri(uri):
# other sorts of information that scare users but are essential for
# debugging

def rospydebug(msg, *args):
"""Internal rospy client library debug logging"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The information that these functions are "internal" is being lost in the patch. Based on the fact that the functions are intended to be internal I am wondering why you want to allow kwargs here? Anyway if it makes still sense to allow them here please update the patch to add them to the existing function definitions and pass them along.

@mgrrx
Copy link
Contributor Author

mgrrx commented Sep 21, 2018

My intention here was to remove code that basically does nothing and at the same time prevents to pass additional keyword arguments. I stumbled upon it when implementing #1289
Rationale: The python logging module by default inspects two keyword arguments: exc_info and extra. I found it quite helpful in the past to pass exc_info and I think it would not hurt to use it e.g. in https://github.com/ros/ros_comm/blob/melodic-devel/clients/rospy/src/rospy/impl/tcpros_base.py#L567

@mgrrx mgrrx changed the base branch from lunar-devel to melodic-devel September 21, 2018 19:41
@dirk-thomas
Copy link
Member

The information about the "internal" nature of the API is still lost from the documentation. Before this note appeared in the docblock of the function.

To enable kwargs put keep the docblock you could do something like this:

def rospydebug(msg, *args, **kwargs):
    """Internal rospy client library debug logging"""
    _rospy_logger.debug(msg, *args, **kwargs)

@mgrrx
Copy link
Contributor Author

mgrrx commented Sep 22, 2018

changed as suggested. I also fixed the wrong docstring

@dirk-thomas
Copy link
Member

Please rebase the patch to resolve the conflicts.

@mgrrx mgrrx force-pushed the kwargs-for-internal-logging branch from 5910331 to 540e787 Compare February 7, 2020 18:19
@mgrrx
Copy link
Contributor Author

mgrrx commented Feb 7, 2020

done

@dirk-thomas
Copy link
Member

@ros-pull-request-builder retest this please

@dirk-thomas
Copy link
Member

Thanks for the patch.

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

Successfully merging this pull request may close these issues.

None yet

2 participants