Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port to ROS 2 #45

Closed
wants to merge 24 commits into from
Closed

Conversation

vmayoral
Copy link

Direct to ros2 branch.

@vmayoral
Copy link
Author

@rhaschke, any chance you can give a hand here pointing to the ros2 branch? I'll look at the conflicts ASAP.

@rhaschke rhaschke changed the base branch from melodic-devel to ros2 February 19, 2019 21:13
@rhaschke
Copy link
Contributor

ros2 branch created.

@vmayoral
Copy link
Author

Solved conflicts, addressed some minor issues.

@rhaschke and @davetcoleman can you please merge this?

CMakeLists.txt Outdated
# if(BUILD_TESTING)
# find_package(rostest REQUIRED)
# add_rostest(test/srdf_parser.test)
# endif()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should never disable tests, the likely hood that someone comes back and re-enables this is low due to human nature. tests are our friends

* ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
* POSSIBILITY OF SUCH DAMAGE.
*********************************************************************/
* Software License Agreement (BSD License)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please revert this types of changes, they are noise

@@ -41,7 +41,7 @@
#include <string>
#include <vector>
#include <utility>
#include <urdf/model.h> // TODO: replace with urdf_model/types.h in Lunar
#include <urdf/model.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this TODO by @rhaschke should be addressed per the original PR:
#27

package.xml Outdated
<name>srdfdom</name>
<version>0.5.1</version>
<description>Parser for Semantic Robot Description Format (SRDF).</description>
<author email="[email protected]">Ioan Sucan</author>
<author email="[email protected]">Guillaume Walck</author>
<author email="[email protected]">Víctor Mayoral Vilches</author>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think its fair to claim ownership over a package just because you ported it to ROS 2. for example, i have 5 commits and @rhaschke 17 but we haven't made ourselves authors of the package.

feel free to list yourself as a maintainer though!

<name>srdfdom</name>
<version>0.5.1</version>
<description>Parser for Semantic Robot Description Format (SRDF).</description>
<author email="[email protected]">Ioan Sucan</author>
<author email="[email protected]">Guillaume Walck</author>
<author email="[email protected]">Víctor Mayoral Vilches</author>

<maintainer email="[email protected]">Dave Coleman</maintainer>maintainer>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

im happy to be removed as a maintainer. notably, the XML appears broken for some reason

src/model.cpp Outdated
* LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN
* ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
* POSSIBILITY OF SUCH DAMAGE.
*********************************************************************/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please avoid these changes in the future

@@ -1,36 +1,36 @@
/*********************************************************************
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert this file, there are no changes

package.xml Outdated
<build_depend>boost</build_depend>
<build_depend>cmake_modules</build_depend>
<build_depend>libconsole-bridge-dev</build_depend>
<build_depend>urdf</build_depend>
<build_depend>urdf_model</build_depend>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This package is not released in melodic or crystal. I believe it should be reverted

@mlautman mlautman mentioned this pull request Mar 13, 2019
@mlautman mlautman changed the base branch from ros2 to crystal-devel March 14, 2019 21:29
@mlautman
Copy link

Unless @vmayoral has any objections, I would like to close this in favor of #48

@rhaschke rhaschke closed this Nov 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants