-
Notifications
You must be signed in to change notification settings - Fork 45
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
Enforce ASCII characters on string and char fields #26
Conversation
d58e0a5
to
ac4aa82
Compare
@dirk-thomas I believe you're the best person to review this. Not sure how this plays with (or if it's already fixed in) the refactored IDL code. |
The patch looks fine to me. Please run CI for the change and maybe add a test to check what happens when passing non-ASCII strings to it. |
@dirk-thomas I think that writing a test for this is going to be somewhat involved. The conversions only take place within rclpy C extension code. I'd have to call publish on a message with a non-ascii string field to trigger a check when going from Python to C, and then take a message with a non-ascii string as sent by code that doesn't have such validations (e.g. a C++ node) to trigger a check when going from C to Python. Maybe a candidate for a |
That part sounds fairly straight forward?
If the test only covers the Python-to-C case but not this one that would be sufficient for me. |
That's the easy part, yes. If you're OK with just testing Python-to-C, so am I. But I think we could add the test in |
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.
👍 with passing CI.
CI's green now! We're good to go. |
This pull request adds ASCII codification and de-codification steps whenever dealing with string and char message fields.
Fixes ros2/ros2cli#176 (which is actually one instance of an issue that is Python support wide).