Align the nsecs field of the timestamps. #45
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -121,7 +121,7 @@ def strify_message(val, indent='', time_offset=None, current_time=None, field_fi | |||
| nsec_str = '\n%snsecs: ' % indent + (format_str % val.nsecs) | |||
| return sec_str + nsec_str | |||
| else: | |||
| return '\n%ssecs: %s\n%snsecs: %s'%(indent, val.secs, indent, val.nsecs) | |||
| return '\n%ssecs: %s\n%snsecs:% 10d'%(indent, val.secs, indent, val.nsecs) | |||
There was a problem hiding this comment.
Would this make more sense: nsecs: %9d
|
I completely agree, your suggestion more clearly explains the intent of the formatting. |
|
The change looks good to me now. Thanks for the PR and updating it. Please squash the changes into a single commit before the merge. |
This change ensures that the nsecs field is always right aligned such that the least significant digit of the nsecs field aligns with the least significant digit of the secs field. This makes it easier to inspect how far into the second the time has progressed. The format specification explains the intent; always one space and aligned to the right with a minimum width of 9 characters.
iwanders
force-pushed
the
improvement_nsecs_alignment
branch
from
838ea5b
to
edb9c5a
Compare
|
This should be it. |
|
Great. Thanks. |
dirk-thomas
added a commit
that referenced
this pull request
Dec 28, 2015
Align the nsecs field of the timestamps.
|
Due to our change the regression test fails (at this test: test_genpy_message.py#L568), would you like me to add a commit that introduces the spaces to fix this? |
|
I noticed the failing test after merging. If you could create a new PR which fixes the problem that works be great. Thanks. |
iwanders
added a commit
to iwanders/genpy
that referenced
this pull request
Dec 31, 2015
The alignment of the nsecs field was changed in ros/genpy@edb9c5a and merged with pull request ros#45, this caused the regression test for the timestamp field to fail. This commit updates the comparison value of the regression tests to reflect the new alignment.
iwanders
added a commit
to iwanders/genpy
that referenced
this pull request
Dec 31, 2015
The alignment of the nsecs field was changed in ros/genpy@edb9c5a (pull request ros#45), this caused the regression test for the timestamp field to fail. This commit updates the comparison value of the regression tests to reflect the new alignment.
iwanders
added a commit
to iwanders/genpy
that referenced
this pull request
Dec 31, 2015
The alignment of the nsecs field was changed in ros/genpy@edb9c5a (pull request ros#45), this caused the regression test for the timestamp field to fail. This commit updates the comparison value of the regression tests to reflect the new alignment.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change ensures that the nsecs field is always right aligned such that the least significant digit of the secs field aligns with the least significant digit of the secs field. This makes it easier to inspect how far into the second the time has progressed.
Before:
After:
A minor aesthetic change, but this makes it easier to look for the start of a certain second, as well as easier to read because the values of the same significance are always on the same position.
Hopefully it doesn't break anyone's scripts relying on the output; there still is always a space after the semicolon, but below the 100ms there are multiple spaces between the semicolon and the number.