remove use of hacks

[wjwwood]

This hack:

https://github.com/osrf/capabilities/blob/master/scripts/capability_server#L5-L22

Can be removed since rospy version 1.9.50 is now out:

http://www.ros.org/debbuild/hydro.html?q=rospy http://www.ros.org/debbuild/groovy.html?q=rospy

We should also investigate making an upstream fix/change for this hack:

https://github.com/osrf/capabilities/blob/master/src/capabilities/server.py#L91-L116

[esteve]

I added a new argument to rospy.Service (in ros_comm) so the error handling code can be overriden to suppress the rospy.logerr output. See https://github.com/esteve/ros_comm/compare/custom-error-handler

Though I don't know if that was the sole reason for patching ServiceImpl, would that change in rospy be enough?

[coveralls]

Coverage remained the same when pulling ef63a551c7cd012fb1a61471f3d5184e1e659f1a on esteve:remove-hacks-41 into f065699a5da1161728c6c0b66d614dde667ccc5f on osrf:master.

[wjwwood]

Interesting that the coverage went down. I wonder if the patch to rospy is not sufficient. I am pretty sure that I tested it though. Can you look into why the coverge went down?

[esteve]

Sure, no problem. src/capabilities/server.py has fairly low coverage (31%), I'll see if I can improve it a bit.

[wjwwood]

Specifically src/capabilities/server.py has 100% coverage in the master branch. My first thought is that somehow coverage is not getting reported for things in rospy callbacks (which is what this hack fixed: https://github.com/osrf/capabilities/blob/master/scripts/capability_server#L5-22).

[esteve]

@wjwwood Okay, I must have read the Coveralls and Travis output wrong. The change in the Service constructor signature makes the code in this branch incompatible with the released version of rospy, and thus the tests never run past the first call to the Service constructor (see https://coveralls.io/files/108492233#L281).

Also, Travis shouldn't report that the build was correct despite the errors (https://travis-ci.org/osrf/capabilities/builds/16155234#L1321), I'll make it a bit more robust.

[wjwwood]

Yes, there looks to be a false positive there.

[coveralls]

Coverage decreased (-7.59%) to 92.41% when pulling 150c4a40ababaaee91c77ed420a39e96700d3eff on esteve:remove-hacks-41 into f065699a5da1161728c6c0b66d614dde667ccc5f on osrf:master.

[coveralls]

Coverage remained the same when pulling 150c4a40ababaaee91c77ed420a39e96700d3eff on esteve:remove-hacks-41 into f065699a5da1161728c6c0b66d614dde667ccc5f on osrf:master.

[wjwwood]

It is weird that this is still not getting back the coverage, it is like the fix in rospy was not sufficient to replace the hack we had in there. @esteve can you look into this, testing the coverage from source on the latest version of rospy's debs for hydro?

[wjwwood]

@esteve bump, lets try to resolve this by early next week if possible, I am ramping up for a 0.1.0 release soon and I would like to have these out by then.

[esteve]

@wjwwood I created a pull request for ros_comm with the changes for defining custom handlers for errors [1]. Once we merge that, we can merge this pull request.

1 - https://github.com/ros/ros_comm/pull/375

[esteve]

Waiting for ros/ros_comm to be released before merging this branch.