Let all imports come from toplevel module, as one line (#16)

That way instead of

```
from launch_frontend_py import launch
from launch_frontend_py.actions import executable
```

A user only has to remember a single module import

```
from launch_frontend_py import launch, executable
```

Collapsed the contents of `action.py` in to `__init__.py` to simplify the handling of `__all__` for that toplevel module.
Added `make_valid_name` to handle the underscore suffixing, so it's done the same way both places it's used.
Made the import unit tests a bit more complete.

Signed-off-by: Emerson Knapp <[email protected]>

Author: emersonknapp

README.md

@@ -22,8 +22,7 @@ The available actions and substitutions are identical to XML and YAML launchfile
 Here is an example launchfile with a few features:
 
 ```py
-from launch_frontend_py import launch
-from launch_frontend_py.actions import arg, executable
+from launch_frontend_py import arg, executable, launch
 
 
 def generate_launch_description():

launch_frontend_py/__init__.py

@@ -12,22 +12,88 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-from typing import List
+from typing import Any, Callable, List
 
 from launch.frontend.parser import Parser
 from launch.launch_description import LaunchDescription
 
-from . import actions
-from .entity import Entity
+from .entity import Entity, make_valid_name
+
+__all__ = [
+    'launch',
+]
 
 
 def launch(actions: List[Entity]) -> LaunchDescription:
+    """Entrypoint of launchfile, produce a LaunchDescription of the provided entities."""
     parser = Parser()
     root_entity = Entity('launch', children=actions)
     return parser.parse_description(root_entity)
 
 
-__all__ = [
-    'launch',
-    'actions',
-]
+def __make_action_factory(action_name: str) -> Callable[..., Any]:
+    """Create a factory function that constructs an Entity of the given action type."""
+    action_name = make_valid_name(action_name)
+
+    def fn(*args, **kwargs):
+        return Entity(action_name, *args, **kwargs)
+
+    fn.__doc__ = f'launch_frontend_py action: {action_name} (dynamically generated)'
+    fn.__name__ = action_name
+    fn.__qualname__ = action_name
+    fn.__module__ = __name__
+    globals()[action_name] = fn
+    __all__.append(action_name)
+    return fn
+
+
+def __getattr__(name: str) -> Any:
+    """
+    Dynamically create action factory functions on demand.
+
+    This is called in `from actions import <name>`, `getattr(actions, <name>)`, or `actions.<name>`
+    """
+    import importlib
+    from launch.frontend import Parser
+    from launch.frontend.expose import action_parse_methods
+
+    if name in __all__:
+        # It's already been loaded, just return it
+        return importlib.import_module(f'.{name}', __name__)
+
+    # Not loaded here yet, perhaps in an extension that wasn't imported yet
+    Parser.load_launch_extensions()
+
+    if name in action_parse_methods:
+        return __make_action_factory(name)
+    elif name.endswith('_'):
+        # The name has a trailing underscore, which we add to actions with reserved Python names
+        # Try again without the underscore
+        try:
+            return __getattr__(name[:-1])
+        except AttributeError as e:
+            raise AttributeError(f'{e} (or "{name}")')
+    else:
+        # It's not registered, raise the usual error
+        raise AttributeError(f'module {__name__} has no attribute "{name}"')
+
+
+def __preseed_all_actions() -> None:
+    """
+    Pre-seed all available actions into this module.
+
+    This preloads all launch extensions and fills __all__ with available actions.
+
+    This is not necessary, since __getattr__ loads actions on demand,
+    but it allows tools and users to see all available actions in __all__.
+    """
+    from launch.frontend.expose import action_parse_methods
+    from launch.frontend import Parser
+    Parser.load_launch_extensions()
+
+    action_names = list(action_parse_methods.keys())
+    for action_name in action_names:
+        __getattr__(action_name)
+
+
+__preseed_all_actions()

launch_frontend_py/actions.py

@@ -13,7 +13,9 @@
 # limitations under the License.
 
 """Module for launch_frontend_py Entity class."""
+import builtins
 from collections.abc import Iterable
+import keyword
 from typing import (
     List,
     Optional,
@@ -30,7 +32,25 @@
     is_instance_of,
 )
 
-from .util import is_reserved_identifier
+
+def is_reserved_identifier(name: str) -> bool:
+    """
+    Check if a name is a reserved identifier in Python.
+
+    Used to avoid naming issues in the launch DSL that overlap with Python reserved words.
+    """
+    return keyword.iskeyword(name) or name in dir(builtins)
+
+
+def make_valid_name(name: str) -> str:
+    """
+    Make a valid Python identifier for an action or attribute name.
+
+    If the name is a reserved identifier in Python, append an underscore to it.
+    """
+    if is_reserved_identifier(name):
+        return f'{name}_'
+    return name
 
 
 class Entity(BaseEntity):
@@ -108,9 +128,7 @@ def get_attr(  # type: ignore[override]
         See :py:meth:`launch.frontend.Entity.get_attr`.
         Does not apply type coercion, only checks if the read value is of the correct type.
         """
-        # Reserved identifiers are all suffixed with an underscore
-        if is_reserved_identifier(name):
-            name += '_'
+        name = make_valid_name(name)
 
         if name not in self.__attrs:
             if not optional:

launch_frontend_py/entity.py

@@ -12,8 +12,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-from launch_frontend_py import launch
-from launch_frontend_py.actions import arg, executable
+from launch_frontend_py import arg, executable, launch
 
 
 def generate_launch_description():

launch_frontend_py/util.py

@@ -14,8 +14,7 @@
 
 from pathlib import Path
 
-from launch_frontend_py import launch
-from launch_frontend_py.actions import arg, executable, group, include, let, log, set_env
+from launch_frontend_py import arg, executable, group, include, launch, let, log, set_env
 
 THIS_DIR = Path(__file__).parent
 

test/launch/basic_launch.py

@@ -20,4 +20,4 @@ def test_action_list_preseed():
     # already filled where possible without explicit access
     # picking a representative subset of actions (that are present in Humble+)
     for x in ['arg', 'timer', 'executable', 'let', 'group', 'include', 'set_env']:
-        assert x in launch_frontend_py.actions.__all__
+        assert x in launch_frontend_py.__all__

test/launch/include_launch.py

@@ -14,7 +14,7 @@
 
 from launch.action import Action
 from launch.frontend import expose_action
-from launch_frontend_py import actions
+import launch_frontend_py as actions
 import pytest
 
 
@@ -32,24 +32,47 @@ def parse(cls, entity, parser):
 class DynamicCreationTest(Action):
     """Test action that exposes an action after first import."""
 
+    def __init__(self, value, **kwargs) -> None:
+        super().__init__(**kwargs)
+        self.value = value
+
     @classmethod
     def parse(cls, entity, parser):
         _, kwargs = super().parse(entity, parser)
+        kwargs['value'] = kwargs.get('value', None)
         return cls, kwargs
 
 
-def test_dynamic_attrs():
-    name = actions.let.__name__
-    assert name == 'let'
-    str_repr = str(actions.let)
+def test_action_getattr():
+    group = actions.group
+    assert group.__name__ == 'group'
+    test_group = group()
+    assert test_group.type_name == 'group'
+
+
+def test_action_import():
+    from launch_frontend_py import let
+    assert let.__name__ == 'let'
+    str_repr = str(let)
     assert str_repr.startswith('<function let')
 
+    x = let(name='test_name', value='test_value')
+    assert x.get_attr('name') == 'test_name'
+    assert x.get_attr('value') == 'test_value'
+
+
+def test_invalid_action_raise():
     with pytest.raises(AttributeError):
         getattr(actions, 'non_existent_action')
 
-    test_group = actions.group()
-    assert test_group.type_name == 'group'
+    with pytest.raises(AttributeError):
+        _ = actions.other_nonexistent
+
+    with pytest.raises(ImportError):
+        import launch_frontend_py.non_existent_import  # noqa: F401
 
+
+def test_dynamic_attrs():
     test_arg = actions.arg(name='argname', default='argvalue')
     assert test_arg.type_name == 'arg'
     assert test_arg.get_attr('name') == 'argname'
@@ -61,14 +84,12 @@ def test_dynamic_attrs():
 
 def test_dynamic_create():
     assert actions.foo is not None
-
     assert actions.foo.__name__ == 'foo'
     assert actions.foo().type_name == 'foo'
-
-    with pytest.raises(AttributeError):
-        actions.node
+    x = actions.foo(value='bar')
+    assert x.get_attr('value') == 'bar'
 
 
-def test_bultin_suffix():
+def test_builtin_suffix():
     assert actions.while_.__name__ == 'while_'
     assert actions.while_().type_name == 'while_'

test/test_action_list_preseed.py

@@ -12,7 +12,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-from launch_frontend_py.actions import group, log
+from launch_frontend_py import group, log
 import pytest