evaluation of expressions in roslaunch#762
evaluation of expressions in roslaunch#762rhaschke wants to merge 8 commits intoros:indigo-develfrom
Conversation
- removed redundant arg py_if_test (equals if_test) - test implicit argument usage in $(eval ...)
|
@gerkey Can you please review this. Due to the advanced time I think we should not merge this into Jade but into Kinetic only. If it works there it can always be backported. |
|
@rhaschke This looks good, thanks. I'm torn about the shortcut on args. It's certainly less typing, but should we be worried about name collisions with other symbols in Python? If there's any risk of the automatic substitution doing the wrong thing, I'd rather just force people to write Other than that issue, +1 from me. The implementation is clear and the test suite is good. @dirk-thomas Agreed on targeting kinetic now, and considering a backport later once we have more experience with it. There's one failure on Jenkins with this branch, but it looks very unrelated: http://build.ros.org/job/Kpr__ros_comm__ubuntu_xenial_amd64/14/testReport/test_bag/TestRosbag/test_get_compression_info/. Is that a known flaky test? |
- showing precedence of substitution functions over args - redefinition of builtin symbols
|
|
||
| def __getitem__(self, key): | ||
| try: | ||
| return self._functions[key] |
There was a problem hiding this comment.
substitution functions take precedence over implicit argument usage
|
@gerkey Indeed the automatic substitution might overwrite existing symbols. However, evaluation of substitions functions (arg, anon, find, etc) already takes precedence over implicit argument usage (as pointed out above and verified with an additional unittest).
I would prefer the latter option, which I already implemented in 1c9d7a0. |
|
lgtm Can you please re-target for kinetic and then we'll consider it for merging? |
|
Retargeting for kinetic |
|
Is there any chance of this feature being backport to indigo? |
|
Larger feature like this is commonly not backported since the chance of regressions is outweighing the desire for stability for a ROS distro which has been released for over 2.5 years. |
|
We're using very ugly work-arounds for this missing feature at the robotics startup I'm working at. If it means anything, I'd really like to have this feature on our system without having to move to Kinetic. String comparisons would make really be a big help to make this project more maintainable. |
This replaces PR #707, providing a first proof-of-concept implementation of a generic $(eval ...) substitution arg as discussed in #707.
Unfortunately, the new resolve method cannot be seamlessly integrated into the existing
resolve_argsmechanism (as suggested by @gerkey), because the existing code uses the simple parser in_collect_args, which cannot handle parentheses within$(xxx )expressions. Hence, I handle$(eval ...)expressions in a specific fashion before resorting to the other resolve args.For a similar reason, $(eval ) substitution args can only be handled if they span the whole string.
The branch https://github.com/ubi-agni/ros_comm/tree/roslaunch-eval provides a basic implementation.
The extension branch https://github.com/ubi-agni/ros_comm/tree/implicit-arg-access comprises a simplification to allow implicit access to arg values. Hence, instead of writing
$(eval arg('my_arg') == 42)one can directly write:
$(eval my_arg == 42)What do you think about that implementation?