Corrected with PR comments

Signed-off-by: ivanpauno <[email protected]>

launch_frontend/launch_frontend/parser.py

@@ -14,6 +14,9 @@
 
 """Module for Parser methods."""
 
+from typing import Optional
+from typing import Text
+
 import launch
 
 from .entity import Entity
@@ -28,32 +31,45 @@ def str_to_bool(string):
     raise RuntimeError('Expected "true" or "false", got {}'.format(string))
 
 
+def load_optional_attribute(
+    kwargs: dict,
+    entity: Entity,
+    name: Text,
+    constructor_name: Optional[Text] = None
+):
+    """Load an optional attribute of `entity` named `name` in `kwargs`."""
+    attr = getattr(entity, name, None)
+    key = name
+    if constructor_name is not None:
+        key = constructor_name
+    if attr is not None:
+        kwargs[key] = attr
+
+
 def parse_executable(entity: Entity):
     """Parse executable tag."""
     cmd = entity.cmd
-    cwd = getattr(entity, 'cwd', None)
-    name = getattr(entity, 'name', None)
-    shell = str_to_bool(getattr(entity, 'shell', 'false'))
-    prefix = getattr(entity, 'launch-prefix', None)
-    output = getattr(entity, 'output', 'log')
-    args = getattr(entity, 'args', None)
-    args = args.split(' ') if args else []
-    if not isinstance(args, list):
-        args = [args]
+    kwargs = {}
+    load_optional_attribute(kwargs, entity, 'cwd')
+    load_optional_attribute(kwargs, entity, 'name')
+    load_optional_attribute(kwargs, entity, 'launch-prefix', 'prefix')
+    load_optional_attribute(kwargs, entity, 'output')
+    shell = getattr(entity, 'shell', None)
+    if shell is not None:
+        kwargs['shell'] = str_to_bool(shell)
     # TODO(ivanpauno): How will predicates be handle in env?
     # Substitutions aren't allowing conditions now.
     env = getattr(entity, 'env', None)
     if env is not None:
         env = {e.name: e.value for e in env}
-
+        kwargs['additional_env'] = env
+    args = getattr(entity, 'args', None)
+    args = args.split(' ') if args else []
+    if not isinstance(args, list):
+        args = [args]
     cmd_list = [cmd]
     cmd_list.extend(args)
     # TODO(ivanpauno): Handle predicate conditions
     return launch.actions.ExecuteProcess(
         cmd=cmd_list,
-        cwd=cwd,
-        additional_env=env,
-        name=name,
-        shell=shell,
-        prefix=prefix,
-        output=output)
+        **kwargs)

launch_xml/README.md

@@ -9,7 +9,7 @@ This package provides an abstraction of the XML tree.
 When having an xml tag like:
 
 ```xml
-<tag attr='2'/>
+<tag attr="2"/>
 ```
 
 If the entity `e` is wrapping it, the following two statements will be true:
@@ -20,7 +20,19 @@ e.attr == '2'
 
 As a general rule, the value of the attribute is returned as a string.
 Conversion to `float` or `int` should be explicitly done in the parser method.
-For handling lists, see `Built-in Substitutions` section.
+For handling lists, the `*-sep` attribute is used. e.g.:
+
+```xml
+<tag attr="2,3,4" attr-sep=","/>
+<tag2 attr="2 3 4" attr-sep=" "/>
+<tag3 attr="2, 3, 4" attr-sep=", "/>
+```
+
+```python
+e.tag.attr == [2, 3, 4]
+e.tag2.attr == [2, 3, 4]
+e.tag3.attr == [2, 3, 4]
+```
 
 ### Accessing XML children as attributes:
 
@@ -66,10 +78,3 @@ The attributes are check in the following order:
 ## Built-in substitutions
 
 See [this](https://github.com/ros2/design/blob/d3a35d7ea201721892993e85e28a5a223cdaa001/articles/151_roslaunch_xml.md) document.
-
-Additional substitution, for handling lists:
-
-`$(list [sep=,] a,b,c,d)`
-: Substituted by a python list, splited by the separator that follows `sep=`.
-  Default separator is `,`.
-  This substitution is evaluated instantaneously, and not in a lazy way.

launch_xml/launch_xml/entity.py

@@ -48,7 +48,12 @@ def children(self):
     def __getattr__(self, name):
         """Abstraction of how to access the xml tree."""
         if name in self.__xml_element.attrib:
-            return self.__xml_element.attrib[name]
+            name_sep = name + '-sep'
+            if name_sep not in self.__xml_element.attrib:
+                return self.__xml_element.attrib[name]
+            else:
+                sep = self.__xml_element.attrib[name_sep]
+                return self.__xml_element.attrib[name].split(sep)
         return_list = filter(lambda x: x.tag == name,
                              self.__xml_element)
         return_list = [Entity(item) for item in return_list]

launch_xml/test/launch_xml/list.xml

@@ -0,0 +1,5 @@
+<root> 
+    <tag1 attr="1,2,3" attr-sep=","/>
+    <tag2 attr="1 2 3" attr-sep=" "/>
+    <tag3 attr="1, 2, 3" attr-sep=", "/>
+</root>

launch_xml/test/launch_xml/test_executable.py

@@ -12,7 +12,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-"""Example of how to parse an xml."""
+"""Test parsing an executable action."""
 
 from pathlib import Path
 import xml.etree.ElementTree as ET

launch_xml/test/launch_xml/test_list.py

@@ -0,0 +1,34 @@
+# Copyright 2019 Open Source Robotics Foundation, Inc.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"""Test parsing list attributes."""
+
+from pathlib import Path
+import xml.etree.ElementTree as ET
+
+from launch_xml import Entity
+
+
+def test_list():
+    """Parse tags with list attributes."""
+    tree = ET.parse(str(Path(__file__).parent / 'list.xml'))
+    root = tree.getroot()
+    root_entity = Entity(root)
+    assert root_entity.tag1[0].attr == ['1', '2', '3']
+    assert root_entity.tag2[0].attr == ['1', '2', '3']
+    assert root_entity.tag3[0].attr == ['1', '2', '3']
+
+
+if __name__ == '__main__':
+    test_list()