Rclcpp: Should rclcpp::Parameter throw if trying to stringify an unset parameter?

[brawner]

This is a follow-up issue to a comment left by @clalancette over at https://github.com/ros2/rclcpp/pull/1344.

Currently rclcpp::Parameter will convert an unset parameter into a string if requested (either by to_string or using the << operator). Should this be expected or should it throw some sort of exception?

[clalancette]

Just to be clear, if you have the following code:

rclcpp::Parameter not_set_variant;
std::stringstream ss;
ss << not_set_variant;

What you get as output is:

{"name": "", "type": "not set", "value": "not set"}

That seems really non-intuitive to me. It's kicking the can down the road to users; to be really robust, users would have to check for this magic string anyway.

[ivanpauno]

I don't see anything wrong with stringifying a rclcpp::PARAMETER_NOT_SET parameter variant.
Maybe the "value": "not set" part in the message is weird, but that could be improved.

[clalancette]

I don't see anything wrong with stringifying a rclcpp::PARAMETER_NOT_SET parameter variant.

But its an arbitrary string that doesn't mean anything. Worse, it sort of converts the type into a string, so after stringifying it you can't really tell the difference between a NOT_SET parameter and a STRING parameter that happened to contain the string "not set".

For me, it seems like it should be an error to try to stringify a NOT_SET parameter. The result can only be undefined, and so I think we should throw an exception here.

[fujitatomoya]

I think as long as user can see and tell the difference NOT_SET and STRING after stringifying, that would be no problem. but if it cannot, it would be nice to notify the user application.

[ivanpauno]

In the case of rclcpp::to_string(rclcpp::Parameter & param), which is the example shown before, I don't see a problem as it seems to be a debug stringified version of the parameter where you can perfectly recognize the type:

parameter not set variant ->
{"name": "", "type": "not set", "value": "not set"}

string parameter variant with value "not set" ->
{"name": "", "type": "string", "value": "not set"}

The one that might be a problem is rclcpp::to_string(const ParameterValue & value), which output is:

parameter not set variant ->
not set

string parameter variant with value "not set" ->
not set

IMO, the output of those should be:

parameter not set variant ->
{"type": "not set", "value": "not set"}

string parameter variant with value "not set" ->
{"type": "string", "value": "not set"}

as parameter value is actually a "variant", a "debug" stringified version of it should include the type.
If the user wants to only print the value, they can use ParameterValue::get and handle errors appropriately.