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

Add timestamp formatting for rosconsole #1533

Closed
wants to merge 1 commit into from

Conversation

abrzozowski
Copy link
Contributor

@abrzozowski abrzozowski commented Nov 18, 2018

It's Python part of formatter based on #1458 and related to the ros/rosconsole#22 for Cpp version.

@dirk-thomas
Copy link
Member

@ros-pull-request-builder retest this please

Copy link
Member

@dirk-thomas dirk-thomas left a 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('}')]
Copy link
Member

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:').

Copy link
Contributor Author

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.

Copy link
Contributor Author

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?

Copy link
Member

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).

Copy link
Contributor Author

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]

@abrzozowski abrzozowski changed the base branch from melodic-devel to noetic-devel February 21, 2020 11:00
@abrzozowski abrzozowski changed the base branch from noetic-devel to melodic-devel February 21, 2020 11:05
@abrzozowski
Copy link
Contributor Author

@ros-pull-request-builder retest this please

@dirk-thomas
Copy link
Member

Please keep targeting noetic-devel since changes will only be accepted on the latest branch.

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.

2 participants