moveit / moveit_ros

THIS REPO HAS MOVED TO https://github.com/ros-planning/moveit
69 stars 118 forks source link

[moveit_ros_visualization] joy: Clean installation (alternative to #703) #704

Closed 130s closed 7 years ago

130s commented 8 years ago

A solution to #638 alternative to https://github.com/ros-planning/moveit_ros/pull/703

v4hn commented 8 years ago

Isn't renaming the python module to moveitjoy_module.py just as bad? Is renaming really necessary here? I didn't check, but I think having a binary and a library with the same name around could work..

davetcoleman commented 8 years ago

but I think having a binary and a library with the same name around could work.

That would be nice, but how this PR is currently works for me, too

+1 indigo onward

130s commented 8 years ago

I felt it's confusing to have multiple python files under the same name, no?

v4hn commented 8 years ago

If one of them is a library in lib/python2.7/side-packages/ and one is a binary in lib/${PACKAGE_NAME}/, I don't think this is a problem.

130s commented 8 years ago

Sorry, but I still think having multiple python files under the same specific name is confusing. Years later when I'll forget about this implementation then run into these 2 files, first thought I'd have is "is this a bug???". It won't take long before I'd figure out that content of these 2 files are actually different, but I'd easily waste a few minutes.

And I forgot to ask; why is renaming just as bad?

v4hn commented 8 years ago

Ok.

On Wed, Jul 06, 2016 at 04:06:51AM -0700, Isaac I.Y. Saito wrote:

And I forgot to ask; why is renaming just as bad?

I'm not sure it is, in this case, that's why I phrased it as a question... Renaming the module entails that anyone importing it in their own python script will have to change their code, right? If this is no problem in this case, then the request looks good to me.

davetcoleman commented 8 years ago

Renaming is bad because there are already roslaunch files referencing moveit_joy.py in various robot config packages.

130s commented 7 years ago

@davetcoleman

Renaming is bad because there are already roslaunch files referencing moveit_joy.py in various robot config packages.

No, that won't happen with this PR; this PR changes the name of python module, not script. Since the name of the python script remains the same, this PR will break the consumers' .launch files.

@v4hn

Renaming the module entails that anyone importing it in their own python script will have to change their code, right? If this is no problem in this case, then the request looks good to me.

Got it. And I doubt existing users reference the module part, so renaming it now should be fine.

v4hn commented 7 years ago

Ok, so if anyone complains (most likely nobody will) blame goes to you @130s . :-) merging. I'll also pick it to jade and kinetic.

130s commented 7 years ago

Thanks for the forward-port Michael.

Correction: x this PR will break the consumers' .launch files. o this PR will not break the consumers' .launch files.

davetcoleman commented 7 years ago

:hamster: Thansk @130s for fixing this up!