-
Notifications
You must be signed in to change notification settings - Fork 193
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
Cleanup tf2_tools to be more modern. #351
Conversation
In short: 1. Change it from an ament_cmake -> ament_python project 2. Fix up the dependencies to be more correct 3. Make it a console script Note that this likely still only works on Linux due to its dependency on a subprocess call to dot, but at least it is more modern. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
install_requires=['setuptools'], | ||
zip_safe=True, | ||
author='Wim Meeussen', | ||
maintainer='Chris Lalancette', |
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.
Add me here as a maintainer too ?
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.
As far as I understand, there is no support for multiple maintainers in the setup.py format: https://stackoverflow.com/questions/9999829/how-to-specify-multiple-authors-emails-in-setup-py .
I guess I could add in a comma-separated string of maintainers/email addresses, but I don't think it is totally necessary. Both of our names are in the package.xml, which is good enough for me.
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.
LGTM
I think the extension for The language is an implementation detail not relevant for the end-user. And common practice is to leave it out for nodes. |
I don't have a strong preference, honestly. There is some benefit to leaving it as-is, so we don't break scripts that are already using this. Nevertheless, if you feel strongly about it, feel free to open a PR and we can discuss it there. |
In short: 1. Change it from an ament_cmake -> ament_python project 2. Fix up the dependencies to be more correct 3. Make it a console script Note that this likely still only works on Linux due to its dependency on a subprocess call to dot, but at least it is more modern. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> (cherry picked from commit d5a2bfc)
In short:
Note that this likely still only works on Linux due to its
dependency on a subprocess call to dot, but at least it is
more modern.
Signed-off-by: Chris Lalancette clalancette@openrobotics.org