Conversation
|
rcl_yaml_param_parser/src/parser.c
Outdated
| name_len = strlen(name); | ||
| sep_len = strlen(sep_str); | ||
| tot_len = ns_len + name_len + sep_len + 1U; | ||
| if (ns_len == sep_len) { |
There was a problem hiding this comment.
self review: would be better to check the contents of the current ns, and skip appending if it ends with the separator already
|
(newer CI):
|
|
looking for a review if anyone gets a chance |
sloretz
left a comment
There was a problem hiding this comment.
I think there should be new tests where the first namespace already has a /, or a parameter that has a ..
| tot_len = ns_len + name_len + sep_len + 1U; | ||
| // Check the last sep_len characters of the current NS against the separator string. | ||
| if (strcmp(cur_ns + ns_len - sep_len, sep_str) == 0) { | ||
| // Current NS already ends with the separator: don't put another separator in. |
There was a problem hiding this comment.
Looking at the tests I think the intent was that each token in the namespace would be it's own level in the config. Namespace /foo/bar/baz and node name smith would be described by:
foo:
bar:
baz:
smith:
ros__parameters:
I'm guessing it was not expected that the names could contain / and .. Allowing them does look more convenient though. To support that there is at least one more bug #301. edit: I think that can be a separate PR though
There was a problem hiding this comment.
Yeah, I think that case can be covered if we just iteratively strip off the trailing separators, but I considered it as a further enhancement (since I didn't expect we would fail in that case: #301 (comment))
There was a problem hiding this comment.
by the way, if we're not sure if we want to support the case where there are trailing slashes in general we can modify the change in this commit d311e0c and go back to just making the exception for the root namespace (maybe error in other cases).
|
From what I've seen, the tests only check that files are parsed. I didn't add a test since the files are going to be parsed fine in either case, they just won't apply to the node correctly. I'll take another look if any tests check what would be passed to rcl. |
|
The design document seems to say that a name/namespace should not end with a slash. The implementation does assume that no namespace entry means root namespace. I'm not convinced specifying "/" as a namespace should be supported (at least in the current specification) |
|
can you clarify where you got that a namespace shouldn't end with a slash? I see it for a topic name. I would expect that it's reasonable for someone to request that a node be put in the |
It does refer to I think remapping allows |
|
My understanding was that this section was referring to namespaces (otherwise it would just be a duplicated version of the constraints on the topic names from the beginning of the document with a lot of constraints omitted) If not I think we should update that document so that the namespace section defines the constraints of a namespace and add to the list of valid examples the single Not saying that it shouldn't be possible to pass the |
|
regression test added in 27fe5ac |
|
Consider the parameters file:
Without this PR, the parameters are stored under the FQN of
//talker, so passing the file to a node withros2 run demo_nodes_cpp talker __params:=demo_parameters.yamlwill not set the parameters on the node (ros2 param list /talkeris empty).With this PR,
ros2 param list /talkerreturns the parametersome_intas expected.Without this change, the parameter file will work if the root namespace is left out completely, but in some cases the user may want to specify explicitly that it is in the root namespace. (I am doing that in https://github.com/ros2/launch/pull/138/files#diff-d78062ac1f3a77c95190e470302b7e69R237 where I write the namespace to a file without considering if it is
/nsor just/).