Allow to import whole external modules with transformers (#483)

* Allow to import whole external modulues with transformers

* Apply suggestions from code review

Co-authored-by: Mateusz Nojek <matnojek@gmail.com>

Author: bhirsz

docs/releasenotes/3.6.0.rst

@@ -48,3 +48,36 @@ This release introduces new option ``--load-transformer`` which imports custom t
 It is also possible to pass transformer configuration either using this option or through ``--configure``::
 
     > robotidy -c ExtClass1.py:param=value --load-transformer ExtClass2.py:param2=value test.robot
+
+Load custom transformers from the module
+-------------------------------------------
+
+It is now possible to load transformers from the Python module. Importing transformers from module works similarly
+to how custom libraries are imported in Robot Framework. If the file has the same name as transformer, it will
+be auto-imported. The following command::
+
+    > robotidy --load-transformer CustomClass.py
+
+will load ``class CustomClass`` from the ``CustomClass.py`` file. It's the old behaviour and it will not change.
+
+If the file does not contain class with the same name, Robotidy will load all transformers from the file (using the
+same logic as importing the module).
+
+If you use directory or Python module, Robotidy will check the ``__init__.py`` file inside. By default it will import
+all transformers from the ``__init__.py`` file::
+
+    from robotidy.transformers import Transformer
+
+    from other_file import TransformerB
+
+    class TransformerA(Transformer)
+
+will import ``TransformerB`` and ``TransformerA`` (it doesn't need to be defined in ``__init__.py``, it's enough that it's imported).
+
+The order of defining will be used as execution order. If you want to use different order you can define ``TRANSFORMERS``
+list in the ``__init__.py``::
+
+    TRANSFORMERS = [
+        "TransformerA",
+        "TransformerB"
+    ]

docs/source/external_transformers.rst

@@ -15,9 +15,47 @@ You can use both ``--transform`` and ``--load-transformer`` options to load cust
 is that ``--transform`` works like include and will only run transformers listed with ``--transform``. While ``--load-transformer``
 will run default transformers first and then user transformers.
 
-You can use the same syntax ``robotidy`` is using for developing internal transformers. The name of the file should
-be the same as name of the class containing your transformer. Your transformer should inherit from ``robot.api.parsing.ModelTransformer``
-parent class.
+Importing whole modules
+---------------------------
+
+Importing transformers from module works similarly to how custom libraries are imported in Robot Framework. If the the
+file has the same name as transformer it will be auto imported. For example following import::
+
+    robotidy --load-transformer CustomFormatter.py src
+
+will auto import class ``CustomFormatter`` from the file.
+
+If the file does not contain class with the same name, it is directory, or it is Python module with ``__init__.py`` file
+Robotidy will import multiple transformers. By default it imports every class that inherits from
+``robot.api.api.parsing.ModelTransformer`` or ``robotidy.transformers.Transformer`` and executes them in order they
+were imported.
+
+Following ``__init__.py``:
+
+  .. code-block:: python
+
+    from robotidy.transformers import Transformer
+
+    from other_file import TransformerB
+
+    class TransformerA(Transformer)
+
+will import TransformerB and TransformerA.
+
+If you want to use different order you can define ``TRANSFORMERS`` list in the ``__init__.py``:
+
+  .. code-block:: python
+
+    TRANSFORMERS = [
+        "TransformerA",
+        "TransformerB"
+    ]
+
+Transformer development
+---------------------------
+
+You can use the same syntax ``robotidy`` is using for developing internal transformers. Your transformer should inherit
+from ``robotidy.transformers.Transformer`` or ``robot.api.parsing.ModelTransformer`` parent class.
 
   .. code-block:: python
 

robotidy/transformers/__init__.py

@@ -9,10 +9,11 @@
 then add ``ENABLED = False`` class attribute inside.
 """
 import copy
+import inspect
 import pathlib
 import textwrap
 from itertools import chain
-from typing import Dict, List, Optional
+from typing import Dict, Iterable, List, Optional
 
 try:
     import rich_click as click
@@ -132,6 +133,13 @@ def add_transformer(self, tr):
         else:
             self.transformers[tr.name] = tr
 
+    def get_args(self, *names) -> Dict:
+        for name in names:
+            name = str(name)
+            if name in self.transformers:
+                return self.transformers[name].args
+        return dict()
+
     def transformer_should_be_included(self, name: str) -> bool:
         """
         Check whether --transform option was used. If it was, check if transformer name was used with --transform.
@@ -170,6 +178,9 @@ def validate_config_names(self):
         Assert that all --configure NAME are either defaults or from --transform/--load-transformer.
         Otherwise raise an error with similar names.
         """
+        # TODO: Currently not used. It enforces that every --config NAME is valid one which may not be desired
+        # if the NAME is external transformer which may not be imported.
+        # Maybe we can add special flag like --validate-config that would run this method if needed.
         for transf_name, transformer in self.transformers.items():
             if not transformer.is_config_only:
                 continue
@@ -213,11 +224,12 @@ class TransformerContainer:
     Stub for transformer container class that holds the transformer instance and its metadata.
     """
 
-    def __init__(self, instance, argument_names, spec):
+    def __init__(self, instance, argument_names, spec, args):
         self.instance = instance
         self.name = instance.__class__.__name__
         self.enabled_by_default = getattr(instance, "ENABLED", True)
         self.parameters = self.get_parameters(argument_names, spec)
+        self.args = args
 
     def get_parameters(self, argument_names, spec):
         params = []
@@ -264,23 +276,66 @@ def get_absolute_path_to_transformer(name, short_name):
     return name
 
 
-def import_transformer(name, args, skip):
-    short_name = get_transformer_short_name(name)
-    name = get_absolute_path_to_transformer(name, short_name)
+def load_transformers_from_module(module):
+    classes = inspect.getmembers(module, inspect.isclass)
+    transformers = dict()
+    for name, transformer_class in classes:
+        if issubclass(transformer_class, (Transformer, ModelTransformer)) and transformer_class not in (
+            Transformer,
+            ModelTransformer,
+        ):
+            transformers[name] = transformer_class
+    return transformers
+
+
+def order_transformers(transformers, module):
+    """If the module contains TRANSFORMERS list, order transformers using this list."""
+    transform_list = getattr(module, "TRANSFORMERS", [])
+    if not (transform_list and isinstance(transform_list, list)):
+        return transformers
+    ordered_transformers = dict()
+    for name in transform_list:
+        if name not in transformers:
+            raise ImportTransformerError(
+                f"Importing transformer '{name}' declared in TRANSFORMERS list failed. "
+                "Verify if correct name was provided."
+            ) from None
+        ordered_transformers[name] = transformers[name]
+    return ordered_transformers
+
+
+def import_transformer(name, config: TransformConfigMap, skip) -> Iterable[TransformerContainer]:
+    import_path = resolve_core_import_path(name)
+    short_name = get_transformer_short_name(import_path)
+    name = get_absolute_path_to_transformer(import_path, short_name)
     try:
-        imported_class = IMPORTER.import_class_or_module(name)
-        spec = IMPORTER._get_arg_spec(imported_class)
-        handles_skip = getattr(imported_class, "HANDLES_SKIP", {})
-        positional, named, argument_names = resolve_args(short_name, spec, args, skip, handles_skip=handles_skip)
+        imported = IMPORTER.import_class_or_module(name)
+        if inspect.isclass(imported):
+            yield create_transformer_instance(
+                imported, short_name, config.get_args(name, short_name, import_path), skip
+            )
+        else:
+            transformers = load_transformers_from_module(imported)
+            transformers = order_transformers(transformers, imported)
+            for name, transformer_class in transformers.items():
+                yield create_transformer_instance(
+                    transformer_class, name, config.get_args(name, short_name, import_path), skip
+                )
     except DataError:
         similar_finder = RecommendationFinder()
         similar = similar_finder.find_similar(short_name, TRANSFORMERS)
         raise ImportTransformerError(
             f"Importing transformer '{short_name}' failed. "
             f"Verify if correct name or configuration was provided.{similar}"
         ) from None
+
+
+def create_transformer_instance(imported_class, short_name, args, skip):
+    spec = IMPORTER._get_arg_spec(imported_class)
+    handles_skip = getattr(imported_class, "HANDLES_SKIP", {})
+    positional, named, argument_names = resolve_args(short_name, spec, args, skip, handles_skip=handles_skip)
     instance = imported_class(*positional, **named)
-    return TransformerContainer(instance, argument_names, spec)
+    return TransformerContainer(instance, argument_names, spec, args)
 
 
 def split_args_to_class_and_skip(args):
@@ -373,11 +428,6 @@ def resolve_core_import_path(name):
     return f"robotidy.transformers.{name}" if name in TRANSFORMERS else name
 
 
-def load_transformer(name, args, skip) -> Optional[TransformerContainer]:
-    import_name = resolve_core_import_path(name)
-    return import_transformer(import_name, args, skip)
-
-
 def can_run_in_robot_version(transformer, overwritten, target_version):
     if not hasattr(transformer, "MIN_VERSION"):
         return True
@@ -412,31 +462,30 @@ def load_transformers(
     """Dynamically load all classes from this file with attribute `name` defined in selected_transformers"""
     loaded_transformers = []
     transformers_config.update_with_defaults(TRANSFORMERS)
-    transformers_config.validate_config_names()
     if not force_order:
         transformers_config.order_using_list(TRANSFORMERS)
     for name, transformer_config in transformers_config.transformers.items():
         if not allow_disabled and not transformers_config.transformer_should_be_included(name):
             continue
-        container = load_transformer(name, transformer_config.args, skip)
-        if transformers_config.force_included_only:
-            enabled = transformer_config.args.get("enabled", True)
-        else:
-            if "enabled" in transformer_config.args:
-                enabled = transformer_config.args["enabled"]
+        for container in import_transformer(name, transformers_config, skip):
+            if transformers_config.force_included_only:
+                enabled = container.args.get("enabled", True)
             else:
-                enabled = getattr(container.instance, "ENABLED", True)
-        if not (enabled or allow_disabled):
-            continue
-        if can_run_in_robot_version(
-            container.instance,
-            overwritten=transformers_config.transformer_was_forcefully_enabled(name),
-            target_version=target_version,
-        ):
-            container.enabled_by_default = enabled
-            loaded_transformers.append(container)
-        elif allow_version_mismatch and allow_disabled:
-            setattr(container.instance, "ENABLED", False)
-            container.enabled_by_default = False
-            loaded_transformers.append(container)
+                if "enabled" in container.args:
+                    enabled = container.args["enabled"]
+                else:
+                    enabled = getattr(container.instance, "ENABLED", True)
+            if not (enabled or allow_disabled):
+                continue
+            if can_run_in_robot_version(
+                container.instance,
+                overwritten=transformers_config.transformer_was_forcefully_enabled(name),
+                target_version=target_version,
+            ):
+                container.enabled_by_default = enabled
+                loaded_transformers.append(container)
+            elif allow_version_mismatch and allow_disabled:
+                setattr(container.instance, "ENABLED", False)
+                container.enabled_by_default = False
+                loaded_transformers.append(container)
     return loaded_transformers

tests/atest/transformers/ExternalTransformer/ExternalTransformers/__init__.py

@@ -0,0 +1 @@
+from .custom_class import CustomClass1, CustomClass2, Transformer

tests/atest/transformers/ExternalTransformer/ExternalTransformers/custom_class.py

@@ -0,0 +1,23 @@
+from robot.api import Token
+
+from robotidy.transformers import Transformer
+
+
+class CustomClass1(Transformer):
+    def visit_SettingSection(self, node):
+        node.header.data_tokens[0].value = node.header.data_tokens[0].value.lower()
+        return node
+
+
+class CustomClass2(Transformer):
+    def __init__(self, extra_param: bool = False):
+        self.extra_param = extra_param
+        super().__init__()
+
+    def visit_TestCaseName(self, node):  # noqa
+        """If extra_param is set to True, lower case the test case name."""
+        if not self.extra_param:
+            return node
+        token = node.get_token(Token.TESTCASE_NAME)
+        token.value = token.value.lower()
+        return node

tests/atest/transformers/ExternalTransformer/ExternalTransformersOrdered/__init__.py

@@ -0,0 +1,7 @@
+from .custom_class import CustomClass1
+from .custom_class2 import CustomClass2, ModelTransformer
+
+# ModelTransformed is imported only to verify it will be not recognized as transformer itself
+
+# CustomClass1 lower case, and CustomClass2 upper case of Settings section header
+TRANSFORMERS = ["CustomClass2", "CustomClass1"]

tests/atest/transformers/ExternalTransformer/ExternalTransformersOrdered/custom_class.py

@@ -0,0 +1,7 @@
+from robotidy.transformers import Transformer
+
+
+class CustomClass1(Transformer):
+    def visit_SettingSection(self, node):
+        node.header.data_tokens[0].value = node.header.data_tokens[0].value.lower()
+        return node

tests/atest/transformers/ExternalTransformer/ExternalTransformersOrdered/custom_class2.py

@@ -0,0 +1,7 @@
+from robot.api.parsing import ModelTransformer
+
+
+class CustomClass2(ModelTransformer):
+    def visit_SettingSection(self, node):
+        node.header.data_tokens[0].value = node.header.data_tokens[0].value.upper()
+        return node

tests/atest/transformers/ExternalTransformer/expected/tests_module_load.robot

@@ -0,0 +1,7 @@
+*** settings ***
+Library     KeywordLibrary
+
+
+*** Test Cases ***
+Test
+    Pass

tests/atest/transformers/ExternalTransformer/expected/tests_module_load_configure.robot

@@ -0,0 +1,7 @@
+*** settings ***
+Library    KeywordLibrary
+
+
+*** Test Cases ***
+test
+    Pass