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

Bad interaction between launch frontend and YAML parameters #74

Closed
hidmic opened this issue Sep 19, 2019 · 3 comments
Closed

Bad interaction between launch frontend and YAML parameters #74

hidmic opened this issue Sep 19, 2019 · 3 comments
Labels
bug Something isn't working

Comments

@hidmic
Copy link
Contributor

hidmic commented Sep 19, 2019

Both launch XML and YAML frontends currently coexist with the YAML parsing of substitution text in Node parameters. This has caused us issues in the past (e.g. ros2/launch#226 (comment)), forcing additional quotes as escape mechanisms. At the time we deemed this as a situation we'd rarely find in the field.


Well, as you can see in RobotWebTools/rosbridge_suite#433, specifically on commit RobotWebTools/rosbridge_suite@d466c89, it's not as rare as we thought.

There, I'm forced to surround a * with two ' on each side.

  • Why a * needs this? Because it's later used as a Node parameter and parameters are parsed as YAML, that parses that * as a malformed alias (see PyYAML documentation about aliases).

  • Why two '? The inner pair prevents * from being parsed as a YAML alias, and the outer pair is removed by the frontend for consistency -- otherwise one has to remember that some values given in e.g. XML are parsed using YAML under the hood and some are not.

IMHO the deeper, unresolved issue that #31 partially addressed is that Substitutions in launch are always text and it is up to the receiving end to do any additional parsing, opening the door to things like this.

@dirk-thomas dirk-thomas added the bug Something isn't working label Sep 19, 2019
@dirk-thomas
Copy link
Member

Since this affects how you have to write a launch file I would think this needs to be fixes asap before more packages start using this.

@ivanpauno
Copy link
Member

ivanpauno commented Sep 19, 2019

I think we should have a custom class for a parameter description, instead of using a native type to represent them. There are a bunch of reasons:

  • Conditional inclusion of parameters isn't now possible.
    As I commented here it would be much easier if we have a custom class for that.
  • If we would have a class that could be constructed like:
    Parameter('name', VALUE_HERE, type=int)
    We could avoid all this problem.

Also, we currently don't provide a way of coercing the result of a substitution to another thing that's not an string. So each class is doing a custom thing for doing this conversions (e.g.: for converting from str to bool). I did a comment about "typed substitutions" in ros2/ros2_documentation#302 (comment), though that's not the only solution.

@ivanpauno
Copy link
Member

After #137, the rules used for coercing type after substitutions and by launch frontend are the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants