Python code cleanup #43
Conversation
|
This seems reasonable to me— doesn't look like there are any significant public forks, so I don't see there being major merge issues with it. Is there development you're wanting to do on actionlib which is made easier by having the package style updated? @mikaelarguedas What are your thoughts, as new maintainer? |
|
The goal is to give the package a more consistent style. I found a lot of the logic to be difficult to follow when studying the threading lock use, for example. Not trying to be a PEP8 pedant, but as far as I could tell, there was no consistent style. More generally speaking, we're investigating opportunities to improve the library's behaviour when used in contexts with a lot of threading and/or inconsistent networking situations. Hopefully this PR will make it easier to study and maintain actionlib's Python implementation to be worth the cost of a considerable hit to the blame history, mutation from the original, etc. |
|
It looks good to me. thanks @ablakey for homogenizing the style! |
| ## Updates the statuses of all goals from the information in status_array. | ||
| ## | ||
| ## @param status_array (\c actionlib_msgs/GoalStatusArray) | ||
| def update_statuses(self, status_array): | ||
| live_statuses = [] | ||
| live_statuses = [] # TODO: unused local var. |
There was a problem hiding this comment.
This line can safely be removed.
|
@mikaelarguedas I'm not sure why the test isn't passing. The tests for the Python implementation pass on my local machine. Is this something I need to explore further or an oddity of the buildfarm? |
|
given that the tests passed on jade for ubuntu 14.04 I don't think there is anything wrong on your side. We''l have to investigate on our hand to see why it's not passing on 16.04 |
|
Are we in a holding pattern for something else to be resolved, or is there something we can do to move forward on this PR? I'm prodding the unit tests that don't seem to pass, but I'm hesitant to base branches off branches of forks and go too far down that path. =) Though this isn't explicitly blocking. |
|
Sorry because it was only a cleanup it went low on my priority list. I'll investigate the test failure on Kinetic shortly and will let you know once we're good to move forward. |
|
@ros-pull-request-builder retest this please |
|
It seems that it was due to a bug on the buildfarm which has been fixed since. Thanks for your work and your patience! |
This was introduced in ros#43. It is not actually correct - you can feasibly get feedback here before a new goal is confirmed. See `send_goal()`.... ``` def send_goal(self, goal, done_cb=None, active_cb=None, feedback_cb=None): # destroys the old goal handle self.stop_tracking_goal() ... self.gh = self.action_client.send_goal(goal, self._handle_transition, self._handle_feedback) ``` and of course it will take more time on top of this for the server to actually process the incoming goal and confirm it. Meantime, it may have sent us feedback messages.
* trailing wwhitespace, semicolons and blank lines * fix star imports violations * fix typos + type errors * more whitespace / comment fixup * python 3 compatibility * more star imports * remove unused imports * use isinstance * add todo for SimpleAction test files * alphabetize new import
This PR represents a significant step toward cleaning up the Python implementation of actionlib. I focused on removing unused imports, removing semicolons, spacing, indentation.
I did not clean up line lengths.
I did not clean up any of the docstrings as I don't have experience with Doxygen and don't want to break anything.