Support yaml string tag '!!str'

[Barry-Xu-2018]

Address ros2/rclcpp#1983

Support

ros2 run pkg_name exe_name --ros-args -p 'calib:=!!str yes'

[fujitatomoya]

lgtm with one confirmation for test.

[Barry-Xu-2018]

@clalancette Could you help to review this PR?

[Barry-Xu-2018]

Rebase code

[Barry-Xu-2018]

@clalancette Thanks for your comments.
I updated code.

[clalancette]

This looks good to me with green CI.

[fujitatomoya]

CI:

• Linux
• Linux-aarch64
• Windows

[ivanpauno]

Do we really need this?

Using "yes" or "no" (i.e. between quotes) should be enough.
If the quoted version is being converted to a bool, then that should be fixed.
But the addition of !!str seems unnecessary IMHO.
It's still fine to have it though, but it may be confusing to only support the !!str tag.

[clalancette]

If the quoted version is being converted to a bool, then that should be fixed.

The problem is that there is fundamentally no way to fix it. The YAML specification states that if it sees "yes" or "no", it automatically casts the value to a boolean type. Further, the YAML specification specially allows for !!str, exactly to handle this situation. See the spec in https://yaml.org/spec/1.2.2/ .

The YAML specification actually allows for forcing any type, via something like !!bool, which this isn't supporting. There is an argument to be made that we should support that as well, but I think that !!str is by far the most likely one to be practically needed. I'd leave the rest of it for future work.

[ivanpauno]

The problem is that there is fundamentally no way to fix it. The YAML specification states that if it sees "yes" or "no", it automatically casts the value to a boolean type.

That only applies if the value is unquoted.
e.g.

import yaml

yaml.safe_load("{string_val: 'yes', boolean_val: yes}")

The regex of the specification also only matches unquoted values, see https://yaml.org/spec/1.2.2/#1022-tag-resolution (10.2.2 doesn't support yes as a boolean though, that's from older specs).
That's why supporting !!str seems unnecessary IMO.

The library we're using allows you to check if the value is quoted or not:

rcl/rcl_yaml_param_parser/src/parse.c

Lines 98 to 99 in 6b516f0

	if (style != YAML_SINGLE_QUOTED_SCALAR_STYLE &&
	style != YAML_DOUBLE_QUOTED_SCALAR_STYLE)

We're actually using that for booleans, so I'm not sure if there's something wrong in the logic.

[fujitatomoya]

That only applies if the value is unquoted.

as mentioned ros2/rclcpp#1983 (comment), this is correct.

That's why supporting !!str seems unnecessary IMO.

direct solution against ros2/rclcpp#1983, it would be easier just to add quote for user? at least that what i would do instead of adding !!str tag before string. having !!str in rcl yaml parse is nothing wrong, and probably unnecessary for most users, but someone still might do that since it is in yaml specification tag?

@Barry-Xu-2018 @iuhilnehc-ynos what do you think?

[ivanpauno]

having !!str in rcl yaml parse is nothing wrong, and probably unnecessary for most users, but someone still might do that since it is in yaml specification tag?

Yes, it's also fine to have tags.
It's confusing to only support one tag though.
But it still seems okay.

I wasn't sure if the option of quoting the value was mentioned or not.

[Barry-Xu-2018]

direct solution against ros2/rclcpp#1983, it would be easier just to add quote for user?

Yeah. It is more convenient for users.

but someone still might do that since it is in yaml specification tag?

'!!str' is defined in YAML specification. So user may use it. If YAML tag isn't supported, we should tell users this limitation and workaround way in the user manual.
If tag is supported, we should support all tags defined in YAML specification.

[ivanpauno]

LGTM!

[fujitatomoya]

created follow-up issue on #1026