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

Guard against invalid key names #117

Merged
merged 4 commits into from
Jun 7, 2019
Merged

Guard against invalid key names #117

merged 4 commits into from
Jun 7, 2019

Conversation

jacobperron
Copy link
Member

In particular, guard against keys that only consist of whitespace and '/' characters.
The error message is also improved slightly.

Fixes #113

In particular, guard against keys that only consist of whitespace and '/' characters.
The error message is also improved slightly.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron
Copy link
Member Author

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@mikaelarguedas
Copy link
Member

This was written at the time where node namespace / names conventions were not enforced.
I don't think it's necessarily a good idea /maintainable to extend this custom logic to cover a single new specific case.
Would it be possible to leverage/provide code allowing to validate node namespace/name instead? something like https://github.com/ros2/rmw/blob/master/rmw/include/rmw/validate_node_name.h

@jacobperron
Copy link
Member Author

Good idea. Considering this is a Python package, what do you think about depending on rclpy, which has functions for validating node names and namespaces?

@mikaelarguedas
Copy link
Member

That sounds good 👍

What looks like the ideal outcome would be to have these utilities in a separate package and be used by both rclpy and sros2 (or any other tool needing them).
A commandline argument added here allowing to bypass the check could be valuable as well (imagining people writing pure DDS code and leveraging sros2 to generate their artifacts).

As this is more work (and would require to preserve the rclpy API if fix for Dashing) than directly using the rclpy functions, it may make sense to just do that for now and ticket the remaining items for a later date.

@jacobperron
Copy link
Member Author

jacobperron commented Jun 5, 2019

What's wrong with leaving the validation functions in rclpy? It seems to me that if any other Python tool wants to validate ROS names then the Python ROS client library would be the package to rely on.
I'm not sure what we'd call the new package containing the name validation functions.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron jacobperron added the in review Waiting for review (Kanban column) label Jun 5, 2019
@jacobperron
Copy link
Member Author

jacobperron commented Jun 5, 2019

Now using rclpy functions for name validation.

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@mikaelarguedas
Copy link
Member

It seems to me that if any other Python tool wants to validate ROS names then the Python ROS client library would be the package to rely on.

That's a good point 👍
lgtm as is then

@jacobperron jacobperron merged commit 84c6f97 into master Jun 7, 2019
@delete-merged-branch delete-merged-branch bot deleted the jacob/fix113 branch June 7, 2019 19:03
clalancette pushed a commit that referenced this pull request Jun 12, 2019
In particular, guard against keys that only consist of whitespace and '/' characters.
The error message is also improved slightly.

* Use name validation functions from rclpy

* Add test for key name validation function

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
ryanewel pushed a commit to aws-ros-dev/sros2 that referenced this pull request Jun 12, 2019
In particular, guard against keys that only consist of whitespace and '/' characters.
The error message is also improved slightly.

* Use name validation functions from rclpy

* Add test for key name validation function

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
clalancette pushed a commit that referenced this pull request Jun 12, 2019
In particular, guard against keys that only consist of whitespace and '/' characters.
The error message is also improved slightly.

* Use name validation functions from rclpy

* Add test for key name validation function

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in review Waiting for review (Kanban column)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Guard against creating keys for empty and whitespace only names
3 participants