search

JeroenDM / moveit_opw_kinematics_plugin

An attempt at writing a MoveIt! plugin for opw_kinematics.
http://wiki.ros.org/moveit_opw_kinematics_plugin
Apache License 2.0
7 stars 7 forks source link

Change kinematic parameter names #41

Closed JeroenDM closed 4 years ago

JeroenDM commented 4 years ago

Name change suggested by @gavanderhoorn, what do you think?

In general, I think most of the names in this package are really long, but they're clear and I have no better suggestions :)

gavanderhoorn commented 4 years ago

I'm curious though: where did that first commit come from?

JeroenDM commented 4 years ago

@gavanderhoorn From you're old fork here. :) I wanted to try cherry-picking from a fork. Turns out it's easy.

JeroenDM commented 4 years ago

Looks fine and as we currently distribute the kinematics.yaml in bundles together with the support packages we can easily migrate our customers.

Good that you mentioned this, I'll have to update the kuka_test_resources. But I'm not sure why the tests on travis did not fail... (Locally, I changed them by accident, so the tests succeeded.)

simonschmeisser commented 4 years ago

You seem to load them from this package anyway? <rosparam command="load" file="$(find moveit_opw_kinematics_plugin)/test/kinematics_kuka.yaml" />

JeroenDM commented 4 years ago

You seem to load them from this package anyway? <rosparam command="load" file="$(find moveit_opw_kinematics_plugin)/test/kinematics_kuka.yaml" /

Right, sorry, I'm reading to many things at the same time. I'll first focus on sorting this out.

The external, unreleased test dependencies could be a problem for the release... I'll move this discussion to the release issue.

JeroenDM commented 4 years ago

Ok, I think it would be good if we make the tests consistent with the way the parameters will be loaded in the future, aka, alongside the kinematics.yaml file.

I proposed the changes here.

JeroenDM commented 4 years ago

This pull request, and it's kinetic counterpart #43 should be ready to be (squash?) merged now.

simonschmeisser commented 4 years ago

the history looks a bit strange indeed, maybe you can do a git rebase origin/melodic-devel to clean it up? you'll need to force push this then

JeroenDM commented 4 years ago

I'll try that out.

JeroenDM commented 4 years ago

Magic! It must have been that I left out the origin/ part the previous time I did the rebase. I have to read up on my git skills.

simonschmeisser commented 4 years ago

There is definitely some magic involved with git ... I often retry the same thing when the result looks strange and then it works :wink:

I think like this you can also merge it directly instead of squash merge?

JeroenDM commented 4 years ago

Indeed, the history is to clean to squash now :)

But I'm not sure if I should do a merge commit, or rebase and merge. I fear with the latter everything might explode.... I mean, I don't really know what is best practice.

JeroenDM commented 4 years ago

MoveIt documentation:

single commits -> squash multiple fixup-style commits -> squash multiple clean, interrelated commits -> merge commit multiple clean, independent commits -> rebase and merge

gavanderhoorn commented 4 years ago

You're overthinking it.

If you want to be able to clearly see where commits came from (ie: a PR), then use a regular merge.

If it's a single commit: squash.

Rebase-and-merge is not really something I use.

JeroenDM commented 4 years ago

You're overthinking it.

That might be something I do sometimes...always...at the moment. :)