[jog_arm] Move and rename to moveit_ros/moveit_servo

[AdamPettinger]

Description

This moves the moveit_experimental/jog_arm package and renames it moveit_ros/moveit_servo @AndyZe. There is already an open PR for ROS2 (moveit/moveit2#223) with some discussion.

@tylerjw will help with migration notes through the name change as well as his big recent PR #2103.

Checklist

• Required by CI: Code is auto formatted using clang-format
• Extend the tutorials / documentation reference
• Document API changes relevant to the user in the MIGRATION.md notes
• Create tests, which fail without this PR reference
• Include a screenshot if changing a GUI
• While waiting for someone to review your request, please help review another open pull request to support the maintainers

[codecov]

Codecov Report

Merging #2165 into master will increase coverage by 0.38%.
The diff coverage is 88.60%.

@@            Coverage Diff             @@
##           master    #2165      +/-   ##
==========================================
+ Coverage   57.57%   57.95%   +0.38%     
==========================================
  Files         327      327              
  Lines       25668    25668              
==========================================
+ Hits        14778    14876      +98     
+ Misses      10890    10792      -98     

Impacted Files	Coverage Δ
...oveit_servo/include/moveit_servo/collision_check.h	100.00% <ø> (ø)
...ervo/include/moveit_servo/joint_state_subscriber.h	100.00% <ø> (ø)
...servo/include/moveit_servo/make_shared_from_pool.h	100.00% <ø> (ø)
moveit_ros/moveit_servo/src/servo.cpp	72.95% <81.25%> (ø)
moveit_ros/moveit_servo/src/servo_calcs.cpp	73.70% <88.46%> (ø)
...os/moveit_servo/include/moveit_servo/servo_calcs.h	100.00% <100.00%> (ø)
...veit_servo/include/moveit_servo/servo_parameters.h	100.00% <100.00%> (ø)
...s/moveit_servo/include/moveit_servo/status_codes.h	100.00% <100.00%> (ø)
moveit_ros/moveit_servo/src/collision_check.cpp	80.26% <100.00%> (ø)
...it_ros/moveit_servo/src/joint_state_subscriber.cpp	100.00% <100.00%> (ø)
... and 12 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b8b5784...88021dd. Read the comment docs.

[henningkayser]

Looks good to me overall. b48460b shouldn't be included here, though. I suggest we merge this here (after @AndyZe and @tylerjw approved) and then I'll sync this to ROS2.

[AndyZe]

I tested it and did a quick verification that "jog" and "Jog" don't appear anywhere (except message types, which we can't change here). Looks good! Approved if you remove that last commit, like Henning pointed out.

[AdamPettinger]

b48460b shouldn't be included here, though

Just to clarify @henningkayser, you want me to leave the COLCON_IGNORE file here in the ROS1 package?

[henningkayser]

b48460b shouldn't be included here, though

Just to clarify @henningkayser, you want me to leave the COLCON_IGNORE file here in the ROS1 package?

No, the commits shouldn't include COLCON_IGNORE at all. This repo doesn't include this file so there should be no need to remove it.

[AndyZe]

I think once these commits all get squashed it won't matter, then...

[AdamPettinger]

b48460b shouldn't be included here, though

Just to clarify @henningkayser, you want me to leave the COLCON_IGNORE file here in the ROS1 package?

No, the commits shouldn't include COLCON_IGNORE at all. This repo doesn't include this file so there should be no need to remove it.

One of my earlier commits added it (first done in moveit2), and I removed it at the end. I think Andy is right and squashing these commits together would result in it never being here. If we are going to squash before merging anyway, I think leaving it is ok. Otherwise I can fix the commit that added it, and remove the final commit

[AdamPettinger]

I removed the COLCON_IGNORE from these commits

[henningkayser]

b48460b shouldn't be included here, though

Just to clarify @henningkayser, you want me to leave the COLCON_IGNORE file here in the ROS1 package?

No, the commits shouldn't include COLCON_IGNORE at all. This repo doesn't include this file so there should be no need to remove it.

One of my earlier commits added it (first done in moveit2), and I removed it at the end. I think Andy is right and squashing these commits together would result in it never being here. If we are going to squash before merging anyway, I think leaving it is ok. Otherwise I can fix the commit that added it, and remove the final commit

The commit history is a good documentation here, I would also keep it probably.

[rhaschke]

IMHO we should try to avoid merging PRs like this as a merge-commit and instead squash-merge them. For example, this one has many closely-related commits and some cleanup commits:


• Move moveit_experimental/moveit_jog_arm -> moveit_ros/moveit_jog_arm
• Rename filepath moveit_ros/moveit_jog_arm -> moveit_ros/moveit_servo

...

• Rename internal namespaces and package/project name to moveit_servo
• Internal rename interface class JogArm to Servo
• Internal rename JOG_ARM_STATUS_CODE_MAP to SERVO_STATUS_CODE_MAP
• Internal rename jog_server to servo_server
• Rename a bunch of internals
• Internal rename JogCalcs -> ServoCalcs
• Rename servo_calcs internals

...

• Clang formatting

@tylerjw: As I have seen several such commits from your side recently (e.g. also #2103), please carefully balance a clean vs. detailed history in future. Sometimes, of course, it is important and useful to keep individual commits of a PR and perform a proper merge. In these cases, (request to) cleanup the PR commit history before merging. Examples are unrelated commits (e.g. backports) in a single PR. However, as a rule of thumb, our guidelines state that a single PR should only tackle a single "problem" and thus all related commits should be squash-merged by default (see GitHub Merge Policies).

[felixvd]

Thanks for mentioning it, I was close to bringing it up, too. #2162 in particular is very long (54 commits, 15 for renaming, two identical commits one after the other). I would actually support force-pushing a clean version to melodic-devel in this case because it's so large.