Yarp: White spaces in Network::connect() source/destination

[claudiofantacci]

This issue is about the following lines of code that have been implemented in #1344.
https://github.com/robotology/yarp/blob/f7b4138e9bca9dc43f8c1fbd5e4b5fafbdfe1972/src/libYARP_OS/src/Network.cpp#L288-L292

The result of this control is that if a space is found the connection is not created.
This is fine, but a simple trailing space in a correctly typed port name will be recognized as a wrong port name, thus the connection will not be made.

I was wondering if it would be better to trim spaces, instead of checking occurencies.
Having a look at the tests

checkTrue(Network::connect("/p1","/p2"),"good connect");
checkFalse(Network::connect("/p1","/p3"),"bad connect, not existing destination");
checkFalse(Network::connect("/p1","/p2 /p3"),"bad connect, invalid destination");
checkFalse(Network::connect("/p1 /p2","/p2"),"bad connect, invalid source");

it would still be fine to trim spaces, but the only wrong behaviour you would have is that a port named /p2/p3 or /p1/p2 could be used in place of the correct ones.

I would say that (probably) the best solution is hybrid:

• trim leading and trailing spaces
• trigger and error for any other spaces in the string

[Nicogene]

I agree for the hybrid solution! Here this guy give an implementation with and without boost for removing trailing and heading spaces.

[claudiofantacci]

Nice, should not be too much an overkill.
Let's see what others feel about this 😄
@drdanz @traversaro @pattacini

[traversaro]

:+1:

[drdanz]

I suggest instead that we have a function somewhere to define a valid port name.
A string with a space is not a valid port name.

[drdanz]

By the way I don't want to support the " /p2" syntax because people will start to use it on purpose and they will depend on it, and when we finally realize that it is not such a good idea to support it, it will be too late and we will not be allowed to deprecate it. So for me any patch that allows such a behaviour is a :-1:

[claudiofantacci]

A string with a space is not a valid port name.

We agree on that.
My point was only about trailing and eventually leading spaces.
I don't think that people will use this on purpose, but I see what you mean, it's a valid point.

As a result of this discussion, I would then suggest to add an error message stating that a whitespace was detected and that it is illegal in port names.
The current implementation only says Failure: no way to make connection.
In my experience it was quite difficult to understand that there was a single trailing space at the end of a string.
We could output something like:
Failure to make the connection between %s->%s: illegal whitespace detected in source|destination port name.

[drdanz]

As a result of this discussion, I would then suggest to add an error message stating that a whitespace was detected and that it is illegal in port names.

Agreed 👍

So, I suggest we add a static bool yarp::os::NetworkBase::isValidPortName(const ConstString& name) that can be useful in many cases (we should check if YARP has some secret function already).