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

Handle zero-width string parameters. #72

Merged
merged 2 commits into from
Sep 23, 2019
Merged

Conversation

hidmic
Copy link
Contributor

@hidmic hidmic commented Sep 19, 2019

Precisely what the title says. Loading an empty string results in None when using the yaml module.

Additional integration tests for zero-width parameters are added.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Copy link
Member

@ivanpauno ivanpauno left a 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)
Copy link
Member

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.

Copy link
Contributor Author

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!

@hidmic
Copy link
Contributor Author

hidmic commented Sep 19, 2019

IMO, any hack around it is a bad idea.

That's connected to #74.

Without this patch,

<param name="foo" value=""/>

results in None and evaluate_parameters() raises. You're then forced to do:

<param name="foo" value="''''"/>

for parameters (!) and

<param name="foo" value="''"/>

for everything else.

@ivanpauno
Copy link
Member

results in None and evaluate_parameters() raises. You're then forced to do:

<param name="foo" value="''''"/>
for parameters (!) and

<param name="foo" value="''"/>

I understand that's quite crappy, but:

  • I don't know what <param name="foo" value=""/> mean in ROS 1.
    Does it set a parameter to an empty string, or does it use its default value?
  • We should solve the underlying problem. Thus is, type inference from performed substitutions.
    Currently, each class is doing type conversions in a custom way.
  • We should probably improve the type inference rules we're using in XML frontend.

@hidmic
Copy link
Contributor Author

hidmic commented Sep 19, 2019

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.

@ivanpauno
Copy link
Member

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 anytime 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 <michel@ekumenlabs.com>
@hidmic
Copy link
Contributor Author

hidmic commented Sep 23, 2019

@ivanpauno see f87c20e. CI is in ros2/launch#335.

@hidmic hidmic merged commit b04713d into master Sep 23, 2019
@delete-merged-branch delete-merged-branch bot deleted the hidmic/zero-width-text branch September 23, 2019 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants