[rosgraph] Fix rospy default rosconsole format for consistency with roscpp #879
Conversation
|
+1 It looks like this repo is failing the newly added cmake warnings settings. |
|
The CMake warnings have been addressed by ros/genmsg#65. |
|
@ros-pull-request-builder retest this please |
|
This is not going to pass until |
|
@ros/ros_team What do you think? Should we merge this and make the default behavior the same or not and keep the currently different default in rospy? |
| msg += ' [%f]' % self._get_time() | ||
| msg += ' %s\n' % record_message | ||
| t = time.time() | ||
| msg = msg.replace('${time}', '%f' % t) |
There was a problem hiding this comment.
In the case that simtime is being used, roscpp prints both the wall clock and the sim time, like this:
[INFO] [${walltime}, ${simtime}]: ${message}
Previously Python did:
[INFO] [WallTime: ${walltime}] [${simtime}] ${message}
With the [${simtime}] part being excluded if simtime is not being used.
This patch doesn't work for me, since it no longer prints wall and sim time at the same time, it now prints wall or sim time. This is both different from the current behavior and different from roscpp (which is the goal of this pr as far as I gather).
There was a problem hiding this comment.
This is relevant C++ code for the non-trivial expansion of ${time}:
There was a problem hiding this comment.
Thanks. I think I fixed it.
|
I'm ok with changing the logging format output in Kinetic so that it is consistent with roscpp. We might want to let people know in a high profile way, say with an email to ros-users@, and at that point ask if anyone is depending on the format of the log output in any serious way. I could imagine people who are parsing log output for some purpose like scraping meta data from log files. This hypothetical user would be broken by a change like this. |
|
I think it's worth making the default consistent. In our email we can give the environment variable to set if you need the old behavior. I think it's unlikely that there will be much hard coded to that format since it's only for rospy logging. Roscpp clearly has a different format. Though we should match the time representation as mentioned by @wjwwood |
wkentaro
changed the title
wkentaro
force-pushed
the
rospy-roscpp-rosconsole-format-consistency
branch
from
eca84fb
to
4942c9c
Compare
|
I think I fixed the code following the reviews. Could you please review again? |
|
Please see wkentaro#1 for proposed changes to your PR. |
…ency Rospy roscpp rosconsole format consistency
|
I documented the new unified behavior in http://wiki.ros.org/action/diff/rosconsole?action=diff&rev1=71&rev2=72 @ros/ros_team Does this look ready-to-merge to you? |
|
@dirk-thomas Thanks. |
|
lgtm |
|
Thank you. I have cherry-picked the change in 7be2c06. |
Close #876