Refactor utility functions in ros2bag #358
Conversation
|
@ros2/aws-oncall - please run this CI job
|
| def create_bag_directory(uri: str) -> str: | ||
| """Create a directory.""" | ||
| try: | ||
| os.makedirs(uri) |
There was a problem hiding this comment.
I don't see the exception-case return value used anywhere.
In the successful case this function hits end of scope without returning a value.
Is the intended return type None (and supposed to log) or is it Optional[str]?
There was a problem hiding this comment.
I moved this from the play verb and I believe it's intention is to simply log the result.
There was a problem hiding this comment.
LGTM'd the PR, but the returned value here doesn't actually ever get printed. You can move forward though, if this is maintaining previous behavior
Signed-off-by: Anas Abou Allaban <[email protected]>
Signed-off-by: Anas Abou Allaban <[email protected]>
Signed-off-by: Anas Abou Allaban <[email protected]>
Signed-off-by: Anas Abou Allaban <[email protected]>
Signed-off-by: Anas Abou Allaban <[email protected]>
Signed-off-by: Anas Abou Allaban <[email protected]>
Signed-off-by: Anas Abou Allaban <[email protected]>
Signed-off-by: Anas Abou Allaban <[email protected]>
piraka9011
force-pushed
the
refactor-ros2bag
branch
from
00df2b0 to
01ab7e8
Compare
Edit: Rebuilt with bionic... |
|
Another round with new config and gist
|
|
|
||
|
|
||
| def print_error(string: str) -> str: | ||
| return '[ERROR] [ros2bag]: {}'.format(string) |
There was a problem hiding this comment.
Why reimplementing logging.error?
There was a problem hiding this comment.
I was debating between using logging vs print statements and it appears that the ros2 verb extension needs to return when something goes wrong/finishes.
If we use logging, we'd need to log followed by a return with the same statement which is duplicate work.
If there's a proper approach for returning from a ros2 verb lmk (I'm following what other packages did).
| def check_positive_float(value: float) -> float: | ||
| """Argparse validator to verify that a value is a float and positive.""" | ||
| try: | ||
| fvalue = float(value) |
There was a problem hiding this comment.
seems to contradict your typing? (value is a floating-point value)
There was a problem hiding this comment.
I wasn't sure what to put the type as since this is supposed to be used as an argparse validator. (Is Any valid?)
|
|
||
| uri = args.output or datetime.datetime.now().strftime('rosbag2_%Y_%m_%d-%H_%M_%S') | ||
|
|
||
| if os.path.isdir(uri): | ||
| return "[ERROR] [ros2bag]: Output folder '{}' already exists.".format(uri) | ||
| return print_error("Output folder '{}' already exists.".format(uri)) |
There was a problem hiding this comment.
Consider moving the argument validation in the arpgarse logic if you keep modifying this code. This will take care for you of reporting errors to the users the "proper way". You could just create a function checking if the directory exists, and call strftime to pass a default value as it's built here.
Signed-off-by: Anas Abou Allaban <[email protected]>
|
One more round ...
|
Signed-off-by: Anas Abou Allaban <[email protected]>
|
ament_export_interfaces() is deprecated, use ament_export_targets() instead
NVM, this was addressed in #360 |
ros2bag's API module.Signed-off-by: Anas Abou Allaban [email protected]