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

Conversation

Projects
3 participants
@jacobperron
Copy link
Member

commented Jun 5, 2019

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

Fixes #113

Guard against invalid key names
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

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

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

This comment has been minimized.

Copy link
Member Author

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.

jacobperron added some commits Jun 5, 2019

Use name validation functions from rclpy
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Add test for key name validation function
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Rename directory
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

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

@sloretz

sloretz approved these changes Jun 7, 2019

@jacobperron jacobperron merged commit 84c6f97 into master Jun 7, 2019

1 check passed

DCO DCO
Details

@delete-merged-branch delete-merged-branch bot deleted the jacob/fix113 branch Jun 7, 2019

@clalancette clalancette added this to Needs Backport in Dashing Patch Release 1 Jun 12, 2019

clalancette added a commit that referenced this pull request Jun 12, 2019

Guard against invalid key names (#117)
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 added a commit to aws-ros-dev/sros2 that referenced this pull request Jun 12, 2019

Guard against invalid key names (ros2#117)
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 added a commit that referenced this pull request Jun 12, 2019

Guard against invalid key names (#117)
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 clalancette moved this from Needs Backport to Released in Dashing Patch Release 1 Jun 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.