Add substitution when loading yaml files #1354
Conversation
|
Can you please add a test for this new functionality. |
|
Should I instead of creating a new test yaml file just modify params.yaml? |
|
I was exactly searching for this feature right now! @dirk-thomas do you think you'll backport it also to kinetic-devel when you are happy with the implementation? |
| value = self.resolve_args(value, context) | ||
| self.load_rosparam(context, ros_config, cmd, param, file, value, verbose=verbose) | ||
| subst_function = lambda x: self.resolve_args(x, context) | ||
| self.load_rosparam(context, ros_config, cmd, param, file, value, verbose=verbose, subst_function=subst_function) |
There was a problem hiding this comment.
Without the patch even a plain value (no file) was being substituted when subst_value was set. After this change substitution seems to only happen on the file content. Is that intentional? This seems to change the existing behavior which would be undesired.
There was a problem hiding this comment.
No, I am not changing existing behavior, the code that does the change is moved to
https://github.com/nano-meter/ros_comm/blob/a7ed7d8c003f52757b75121ee021b578b4b33a07/tools/roslaunch/src/roslaunch/loader.py#L405
This way both value and file content substitution is handled the same way.
I also created tests for file and value substitution, so if I introduced undesired changes, those probably would have caught it. If you see a problem, please tell me so I could fix it.
There was a problem hiding this comment.
Thanks for clarifying. I see now this is only relevant when cmd is load where it is handles in the function. The other commands don't mind about value so it is ok to not substitute it in these cases.
That depends if the final patch changes any API or behavior and how big the changes / chances for regressions are. |
The new separate yaml file looks fine to me. Thanks for the patch. Checking CI results before merging... |
|
@ros-pull-request-builder retest this please |
1 similar comment
|
@ros-pull-request-builder retest this please |
|
Since the failing RosAssert unit tests are unrelated (already on the target branch the case and ticketed separately in #1358) I will go ahead and merge this anyway. |
|
Now that this has been merged, I'd like to bring back the issue raised by @alextoind of backporting to kinetic-devel. Is there any chance of that happening @dirk-thomas? |
That will be considered before the next Kinetic release. In general new features have a lower chance of getting backported. |
* Add substitution when loading yaml files like in ros#1051 * Add tests
|
Would also be interested in getting this backported to ROS Kinetic, given that it still has a lifetime of 3 years and this feature looks quite elementary. |
* Add substitution when loading yaml files like in #1051 * Add tests
implements #1051