-
Notifications
You must be signed in to change notification settings - Fork 4
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
Prepare package for public consumption and contributions #2
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good. Only some minor changes
- Get linter tests passing - Remove development crumbs from package.xml and CMakeLists.txt
This test will only work on UNIX-based systems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't agree about the license but I approve these changes.
Weird - ament_copyright was being temporarily weird.
For the most part, these changes are superficial and linter-related.
The most notable exception is that I corrected the namespace delimiter in the parameters from a
/
to a.
, which will need a corresponding update in the OpenRover config files.Once the
serial
package is available in ROS 2, we can think about submitting this as a standard ROS 2 package, at which point we can leverage the ROS 2 buildfarm for CI and PR builds of this repository.These updates make the package buildable on OSX and Windows as well, though the end-to-end smoke test only runs on UNIX-based systems, so it is excluded from Windows testing.
After this is merged, I'd like to tag the first release of the package.