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

Add graph_wrapper to rosdistro #40806

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

hridaybavle
Copy link
Contributor

Package names:

Package Upstream Source:

https://github.com/snt-arg/graph_wrapper.git

Purpose of using this:

Graph wrapper for Networkx library as a tool for lidar_s_graphs.

PR related to lidar_s_graphs ---> #40800

Please Add This Package to be indexed in the rosdistro.

Iron and Humble

The source is here:

https://github.com/snt-arg/graph_wrapper.git

Checks

  • [ x] All packages have a declared license in the package.xml
  • [ x] This repository has a LICENSE file
  • [ x] This package is expected to build on the submitted rosdistro

@github-actions github-actions bot added humble Issue/PR is for the ROS 2 Humble distribution iron Issue/PR is for the ROS 2 Iron distribution labels Apr 24, 2024
@clalancette
Copy link
Contributor

New package review checklist

  • At least one of the following must be present
    • Top level license file:
    • Per package license files:
  • License is OSI-approved: GPLv3
  • License correctly listed in package.xmls
  • Public source repo: https://github.com/snt-arg/graph_wrapper
  • Source repository contains ROS packages
  • Each package meets REP-144 naming conventions - Unfortunately, graph_wrapper is far too generic. It doesn't really describe what it does, and it could mean many things. I'm going to suggest you rename this package to something like situational_graph_wrapper or snt_situational_graph_wrapper, which will make it much clearer.

Besides the above, the sources seem to be missing a setup.cfg file (which will be required to make this package work), and the setup.py has a bunch of TODO comments in it.

@hridaybavle
Copy link
Contributor Author

@clalancette I am looking into the renaming of the repos with my team and will come back with the relevant changes soon.

@clalancette
Copy link
Contributor

@clalancette I am looking into the renaming of the repos with my team and will come back with the relevant changes soon.

Thank you!

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

This now looks good, though I suggest that you update https://github.com/snt-arg/situational_graphs_wrapper/blob/develop/setup.py to have a maintainer and license.

@clalancette
Copy link
Contributor

This needs to be rebased because of the other PRs we put in. Once that is done, we can merge this one.

@hridaybavle
Copy link
Contributor Author

Rebased with master :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
humble Issue/PR is for the ROS 2 Humble distribution iron Issue/PR is for the ROS 2 Iron distribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants