feat (pkg): don't allow hyphen in python pkg name #717
Conversation
Signed-off-by: Fabian Kirschner <fabian.kirschner@gmail.com>
There was a problem hiding this comment.
Are there any instructions for installing from source / executing?
For this specific repository? There aren't any repo-specific steps for ros2cli, but you'd have to follow the regular workflow of creating a workspace to act as an overlay on top of your default ROS workspace: https://docs.ros.org/en/rolling/Tutorials/Workspace/Creating-A-Workspace.html, after which you can build/source/test your workspace with code from your fork of ros2cli.
Should I add a unit test somewhere?
Yes please. Unit tests for the ros2 pkg CLI are here: https://github.com/ros2/ros2cli/blob/master/ros2pkg/test/test_cli.py
| 'choose a different package name.' | ||
| if '-' in args.package_name: | ||
| # Python packages shouldn't include hyphens in their name, refer to PEP-8. | ||
| # Running a via script 'ros2 run' with hyphens in the executable or package name |
There was a problem hiding this comment.
There is a linter warning in CI: Can you remove this trailing whitespace?
| # Running a via script 'ros2 run' with hyphens in the executable or package name | |
| # Running a via script 'ros2 run' with hyphens in the executable or package name |
| # directory the tests for the package go in. | ||
| return "Aborted since 'ament_python' packages can't be named 'test'. Please " + \ | ||
| 'choose a different package name.' | ||
| if '-' in args.package_name: |
There was a problem hiding this comment.
This is one case, but node names with hyphens are also problematic.
ros2 pkg create test_pkg --build-type ament_python --node-name test-node # breaksCan you address this case too?
There was a problem hiding this comment.
Very good catch, I forgot about that 😅
| # Running a via script 'ros2 run' with hyphens in the executable or package name | ||
| # results in errors. | ||
| return "Aborted since 'ament_python' packages can't include hyphens ('-') in " + \ | ||
| "their name (refer to PEP-8). Please choose a different package name." |
There was a problem hiding this comment.
(refer to PEP-8)
Nit: PEP-8 package and module names doesn't explicitly recommend against using hyphens, and no reasoning for this case is provided there either, so I don't know if this is helpful.
|
Thanks for your review, I'll address your points tomorrow. Regarding the pep-8 reference - I thought of that while writing as well.. It's more complicated though because technically calling Should I just remove the pep-8 reference then? |
True, and I don't think it's worth the time trying to make that pathological case work.
👍 |
|
Ok, will do. I won't get to it until next week though |
fixes #715
As @clalancette suggested to block creation of python pkgs with hyphens in the name I didn't implement a warning, but block them by returning early. Similar to what was being done for pkgs names
test.I didn't test this change locally as I didn't find instructions on how to install from source and it's such a minor change. Are there any instructions for installing from source / executing?
Should I add a unit test somewhere?