python evaluation of expressions in roslaunch #784
Conversation
|
+1 The test failures in |
|
Btw, the test failures are indeed unrelated to this change: #787. |
|
@ros-pull-request-builder retest this please |
| @@ -46,6 +46,7 @@ | |||
|
|
|||
| import rosgraph.names | |||
| import rospkg | |||
| import loader | |||
There was a problem hiding this comment.
Is this referring to the relative module roslaunch.loader? Then please use absolute import names (optionally with from and updating the usage) and insert them in alphabetical order. E.g.:
import rosgraph.names
from roslaunch.loader import convert_value
import rospkg
There was a problem hiding this comment.
Aren't relative imports of the folloing form better, since they don't rely on roslaunch referring to the parent module (ie, if the pythonpath has two modules with that name)
from .loader import convert_value
There was a problem hiding this comment.
PEP 8 recommends absolute imports (see https://www.python.org/dev/peps/pep-0008/#imports). If you use the from syntax it needs to be absolute.
(Also I don't think any code within this repo uses relative imports currently?)
There was a problem hiding this comment.
If you use the from syntax it needs to be absolute
Not sure what you mean by that, but thanks for pointing out to me that PEP8 says "Standard library code should avoid complex package layouts and always use absolute imports"
|
Beside the small comments this looks good. Can you please squash the commits after updating. |
|
@dirk-thomas Fixed the PEP8 issues. Shall I squash? It's getting harder to follow the changes then... I guess, it's better when you do during merging. |
|
If I squash and merge using the GitHub interface the single commit won't have any reference to this ticket. Therefore I think it would be better that you squash the commits on this branch and we do a normal merge commit. |
- evaluation of expressions of the form $(eval ...)
- functions env(), optenv(), anon(), find(), arg() map onto corresponding $-substitutions
- implicit access to args using their name, i.e. name instead of arg('name')
d0a6e5e to
98b1829
Compare
|
@dirk-thomas Squashed into a single commit as requested. |
|
Can you please make sure to document this new functionality in the wiki for the Kinetic version of roslaunch (and post the link here). Even though it doesn't require any migration step I think it would also be great to mentioned on this page: http://wiki.ros.org/kinetic/Migration |
|
Would it be desirable to expose |
|
|
I'd say there's a strong argument for making the environment be identical in |
|
python evaluation of expressions in roslaunch
Retargeted #762 to kinetic.