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.

Sign up for GitHub

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

feat (pkg): don't allow hyphen in python pkg name #717

Open
wants to merge 1 commit into
base: rolling
Choose a base branch
from
Open

feat (pkg): don't allow hyphen in python pkg name #717

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 14 additions & 6 deletions ros2pkg/ros2pkg/verb/create.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,12 +140,20 @@ def main(self, *, args):
test_dependencies = ['ament_copyright', 'ament_flake8', 'ament_pep257',
'python3-pytest']

if args.build_type == 'ament_python' and args.package_name == 'test':
# If the package name is 'test', there will be a conflict between
# the directory the source code for the package goes in and the
# 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.'
# Name checks (for python pkgs)
if args.build_type == 'ament_python':
if args.package_name == 'test':
# If the package name is 'test', there will be a conflict between
# the directory the source code for the package goes in and the
# 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:
Copy link
Member

Choose a reason for hiding this comment

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

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 # breaks

Can you address this case too?

Copy link
Author

Choose a reason for hiding this comment

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

Very good catch, I forgot about that 😅

# 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
Copy link
Member

Choose a reason for hiding this comment

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

There is a linter warning in CI: Can you remove this trailing whitespace?

Suggested change
# 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

# 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."
Copy link
Member

Choose a reason for hiding this comment

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

(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.


package = Package(
package_format=args.package_format,
Expand Down