-
Notifications
You must be signed in to change notification settings - Fork 74
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
Handle zero-width string parameters. #72
Conversation
Signed-off-by: Michel Hidalgo <[email protected]>
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.
If you want to yaml-load an empty string from Python, you need to do:
yaml.safe_load("''")
IMO, any hack around it is a bad idea.
@@ -67,7 +67,9 @@ def check_sequence_type_is_allowed(sequence): | |||
if isinstance(value[0], Substitution): | |||
# Value is a list of substitutions, so perform them to make a string | |||
evaluated_value = perform_substitutions(context, list(value)) | |||
yaml_evaluated_value = yaml.safe_load(evaluated_value) | |||
yaml_evaluated_value = yaml.load(evaluated_value) |
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.
why stop using safe load? I don't think that's a good idea.
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.
That's a mistake. A leftover of a previous attempt using a custom Loader for a different reason. Good catch!
That's connected to #74. Without this patch, <param name="foo" value=""/> results in <param name="foo" value="''''"/> for parameters (!) and <param name="foo" value="''"/> for everything else. |
I understand that's quite crappy, but:
|
I fully agree with that. In the meantime, we are not dealing with an empty string, and whether we fail or not depends on what that value ends up being used for. This patch doesn't make things better, but an actual solutoin for #74 won't be ready soon. |
Ok. I don't like this much, but I'll approve after you address my review comment and confirm what ROS 1 is doing in this case. |
Signed-off-by: Michel Hidalgo <[email protected]>
@ivanpauno see f87c20e. CI is in ros2/launch#335. |
Precisely what the title says. Loading an empty string results in
None
when using theyaml
module.Additional integration tests for zero-width parameters are added.