-
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
Add timestamp formatting for rosconsole #1533
Conversation
6713fd4
to
f229193
Compare
f229193
to
3dec6fc
Compare
@ros-pull-request-builder retest this please |
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 add unit tests for the newly introduced feature (as well as the pointed out problematic case).
msg = msg.replace('${walltime}', '%f' % time.time()) # for performance reasons | ||
|
||
while '${walltime:' in msg: | ||
time_format = msg[msg.index('${walltime:') + len('${walltime:'): msg.index('}')] |
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.
What if another placeholder which hasn't been replaced yet precedes the ${walltime:
? msg.index('}')
would return an index which is before msg.index('${walltime:')
.
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.
Ok, you're right. Line:
time_format = msg[msg.index('${walltime:') + len('${walltime:'): msg.index('}')]
should looks like
tag_end_index = msg.index('${walltime:') + len('${walltime:')
time_format = msg[tag_end_index: msg.index('}', tag_end_index)]
I'll fix it by next week.
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.
@dirk-thomas I added unit test for formatted timestamp, but there are some problems on Npr_* builds. Could you tell me what does Npr_ stands for?
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.
These are the PR builds for the upcoming ROS Noetic distribution.
Just recently the default branch of this repo has moved to noetic-devel
so please retarget that branch instead (and then the already existing Mpr results won't be updated anymore).
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.
Is there a problem with a Jenkins pipeline script on line git rev-parse origin/noetic-devel^{commit}
?
12:00:50 > git rev-parse origin/noetic-devel^{commit} # timeout=10
12:00:50 FATAL: Command "git rev-parse origin/noetic-devel^{commit}" returned status code 128:
12:00:50 stdout: origin/noetic-devel^{commit}
12:00:50
12:00:50 stderr: fatal: ambiguous argument 'origin/noetic-devel^{commit}': unknown revision or path not in the working tree.
[Npr__ros_comm__ubuntu_focal_amd64/138/]
I even tried an another PR-draft on #1892, made the same error: FATAL: Command "git rev-parse origin/noetic-devel^{commit}" returned status code 128:
[Npr__ros_comm__ubuntu_focal_amd64/139]
3dec6fc
to
27783bc
Compare
@ros-pull-request-builder retest this please |
27783bc
to
831fe58
Compare
Please keep targeting |
It's Python part of formatter based on #1458 and related to the ros/rosconsole#22 for Cpp version.