search

ros-industrial / motoman

ROS-Industrial Motoman support (http://wiki.ros.org/motoman)
143 stars 194 forks source link

MotoROS - v1.9.11 #550

Closed ted-miller closed 1 year ago

ted-miller commented 1 year ago

Fix for https://github.com/ros-industrial/motoman/issues/492 and https://github.com/ros-industrial/motoman/issues/519. This has been tested.

This PR is a replacement for #520, which was inadvertently closed.

@cjue, thanks for pointing me to your fork.

gavanderhoorn commented 1 year ago

The order of commits confuses me.

2f1631b9 bumps the version to 1.9.11, but doesn't add new binaries.

c2db1475 updates the binaries, but is not the last commit in the PR.

6b6b6d56 is the last commit, changes code, but does not update the binaries, nor bumps the version nr.

@ted-miller: could you please clarify?

ted-miller commented 1 year ago

https://github.com/ros-industrial/motoman/commit/2f1631b9d325451b5c292b4d774fc8fb14797ed3 bumps the version to 1.9.11, but doesn't add new binaries.

In the past, we have handled version 'tagging' with the name of the PR and not necessarily the order of commits. So, in that regard, the PR should really be titled MotoROS - v1.9.11. The merge of the PR served as the final version tag.

I'm now aware that you don't like this work flow, but this was originally implemented back in August. The recent PR was just a copy of the previous commits.

https://github.com/ros-industrial/motoman/commit/c2db1475fc1defc3e4f266bebfe9675ec4dc82d6 updates the binaries, but is not the last commit in the PR.

6b6b6d5 was created in response to feedback/review on the original PR after submitting c2db147.

https://github.com/ros-industrial/motoman/commit/6b6b6d56d04de26f1749f14bb8d5b0ef1cf56f74 is the last commit, changes code, but does not update the binaries

This does actually update binaries. The presumption was that we didn't want to bump the version again just for some feedback on a working PR.

I'll see if I can rearrange history w/o butchering the commits.

gavanderhoorn commented 1 year ago

6b6b6d5 is the last commit, changes code, but does not update the binaries

This does actually update binaries

apologies, I missed it indeed updates the binaries.

If you're ok with having two different versions of the binaries in the commit history without a corresponding version bump, I'm not going to be (any more) annoying (than I already are) about it.

ted-miller commented 1 year ago

If you're ok with having two different versions of the binaries in the commit history without a corresponding version bump

Honestly, it doesn't bother me. When I'm looking at version history, I view the entire PR as a 'work in progress' and don't necessarily review the individual commits that compose the end-result. (Maybe that's just me though)

Personally, I'd vote for merging as is. This has been tested and I don't want to risk breaking it (again). I see you've already updated the title with the version tag.

gavanderhoorn commented 1 year ago

I've not run-tested this.

gavanderhoorn commented 1 year ago

@ted-miller: would you update the ROS wiki (if it's up again already, that is)?

ted-miller commented 1 year ago

will do