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

return an error status on error in rosbag #1257

Merged
merged 1 commit into from Dec 13, 2017

Conversation

Projects
None yet
4 participants
@phil-marble
Copy link
Contributor

commented Dec 13, 2017

I added some sys.exit(1)'s to rosbag commands to help with scripting.
Specifically, I expected a call to rosbag info test.bag.active to
return an error when the bag needed re-indexing.

fixes #1256

@phil-marble

This comment has been minimized.

Copy link
Contributor Author

commented Dec 13, 2017

Hey there.. New here and interested in getting this fix into kinetic...

@wjwwood

This comment has been minimized.

Copy link
Member

commented Dec 13, 2017

@phil-marble it typically takes a few days for someone to have time to review and either merge this or give you feedback on what needs to change. After merging, it will take a few weeks to a month to get into the public .deb files. It might take slightly longer during this part of the year due to people traveling for the holidays.

@phil-marble

This comment has been minimized.

Copy link
Contributor Author

commented Dec 13, 2017

Thanks @wjwwood ! It's not a huge rush, just wondering about process.

I submitted this PR against the default branch (lunar-devel.) How does the org manage cherry-picking to older versions?

@wjwwood
Copy link
Member

left a comment

The change looks reasonable to me.

I only identified this line which might also benefit from using sys.exit instead of return:

https://github.com/phil-marble/ros_comm/blob/c7aee3043fc84fc80b6aae31e5dc7bf4d7a72d53/tools/rosbag/src/rosbag/rosbag_main.py#L298

I think sys.exit make more sense because of how the "cmd" functions are executed:

https://github.com/phil-marble/ros_comm/blob/c7aee3043fc84fc80b6aae31e5dc7bf4d7a72d53/tools/rosbag/src/rosbag/rosbag_main.py#L900

While this line (and few others that follow it) could (probably should) be converted to use sys.exit rather than exit:

https://github.com/phil-marble/ros_comm/blob/c7aee3043fc84fc80b6aae31e5dc7bf4d7a72d53/tools/rosbag/src/rosbag/rosbag_main.py#L493

After a quick scan I didn't see anything else that needed doing related to returning an error code when there's a problem.

I'll leave it up to one of the actual maintainers to review and approve this change however.

@wjwwood

This comment has been minimized.

Copy link
Member

commented Dec 13, 2017

Opening against lunar-devel (or the latest branch) is the right thing to do. The maintainers will back port changes at their discretion or your request.

@@ -449,6 +452,7 @@ def fix_cmd(argv):

print('Try running \'rosbag check\' to create the necessary rule files or run \'rosbag fix\' with the \'--force\' option.')
os.remove(outname)
sys.exit(1)

This comment has been minimized.

Copy link
@dirk-thomas

dirk-thomas Dec 13, 2017

Member

The indentation of this line uses a tab which isn't allowed in Python 3. Please change this to spaces.

@mikepurvis

This comment has been minimized.

Copy link
Member

commented Dec 13, 2017

👍

return an error status on error in rosbag
I added some sys.exit(1)'s to rosbag commands to help with scripting.
Specifically, I expected a call to `rosbag info test.bag.active` to
return an error when the bag needed re-indexing.

@phil-marble phil-marble force-pushed the phil-marble:return_error_on_unindexed branch from c7aee30 to 0df9572 Dec 13, 2017

@phil-marble

This comment has been minimized.

Copy link
Contributor Author

commented Dec 13, 2017

removed the tab

@dirk-thomas

This comment has been minimized.

Copy link
Member

commented Dec 13, 2017

Thank you.

@dirk-thomas dirk-thomas merged commit 44b33aa into ros:lunar-devel Dec 13, 2017

1 check passed

Lpr__ros_comm__ubuntu_xenial_amd64 Build finished.
Details

@phil-marble phil-marble deleted the phil-marble:return_error_on_unindexed branch Dec 15, 2017

dirk-thomas added a commit that referenced this pull request Feb 9, 2018

return an error status on error in rosbag (#1257)
I added some sys.exit(1)'s to rosbag commands to help with scripting.
Specifically, I expected a call to `rosbag info test.bag.active` to
return an error when the bag needed re-indexing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.