Issue OpenCyphal#99 proposal

This is a draft PR to review a proposed change to the public APIs for pydsdl.

This is non-breaking change that adds an optional set of visitors allowing users to implement filters. This also allows for new visitor types to be defined in the future without further API changes.

.gitmodules

@@ -0,0 +1,3 @@
+[submodule "test/public_regulated_data_types"]
+	path = test/public_regulated_data_types
+	url = https://github.com/OpenCyphal/public_regulated_data_types.git

pydsdl/__init__.py

@@ -25,8 +25,8 @@
 _sys.path = [str(_Path(__file__).parent / "third_party")] + _sys.path
 
 # Never import anything that is not available here - API stability guarantees are only provided for the exposed items.
+from .visitors import PrintOutputHandler as PrintOutputHandler # for backwards compatibility
 from ._namespace import read_namespace as read_namespace
-from ._namespace import PrintOutputHandler as PrintOutputHandler
 
 # Error model.
 from ._error import FrontendError as FrontendError

pydsdl/_data_type_builder.py

@@ -2,16 +2,44 @@
 # This software is distributed under the terms of the MIT License.
 # Author: Pavel Kirienko <pavel@opencyphal.org>
 
-from typing import Optional, Callable, Iterable
+import abc
 import logging
 from pathlib import Path
-from . import _serializable
-from . import _expression
-from . import _error
-from . import _dsdl_definition
-from . import _parser
-from . import _data_schema_builder
-from . import _port_id_ranges
+from typing import Callable, Iterable, Optional, Set
+
+from . import _data_schema_builder, _error, _expression, _parser, _port_id_ranges, _serializable
+from .visitors import DsdlFile, NamespaceVisitor
+
+
+class DsdlFileBuildable(DsdlFile):
+    """
+    A DSDL file that can construct a composite type from its contents.
+    """
+
+    @abc.abstractmethod
+    def read(
+        self,
+        lookup_definitions: Iterable["DsdlFileBuildable"],
+        namespace_visitors: Iterable[NamespaceVisitor],
+        print_output_handler: Callable[[int, str], None],
+        allow_unregulated_fixed_port_id: bool,
+    ) -> _serializable.CompositeType:
+        """
+        Reads the data type definition and returns its high-level data type representation.
+        The output should be cached; all following invocations should read from this cache.
+        Caching is very important, because it is expected that the same definition may be referred to multiple
+        times (e.g., for composition or when accessing external constants). Re-processing a definition every time
+        it is accessed would be a huge waste of time.
+        Note, however, that this may lead to unexpected complications if one is attempting to re-read a definition
+        with different inputs (e.g., different lookup paths) expecting to get a different result: caching would
+        get in the way. That issue is easy to avoid by creating a new instance of the object.
+        :param lookup_definitions:              List of definitions available for referring to.
+        :param namespace_visitors:              Namespace visitors to notify about discovered dependencies.
+        :param print_output_handler:            Used for @print and for diagnostics: (line_number, text) -> None.
+        :param allow_unregulated_fixed_port_id: Do not complain about fixed unregulated port IDs.
+        :return: The data type representation.
+        """
+        raise NotImplementedError()
 
 
 class AssertionCheckFailureError(_error.InvalidDefinitionError):
@@ -44,19 +72,21 @@ class MissingSerializationModeError(_error.InvalidDefinitionError):
 class DataTypeBuilder(_parser.StatementStreamProcessor):
     def __init__(
         self,
-        definition: _dsdl_definition.DSDLDefinition,
-        lookup_definitions: Iterable[_dsdl_definition.DSDLDefinition],
+        definition: DsdlFileBuildable,
+        lookup_definitions: Iterable[DsdlFileBuildable],
+        namespace_visitors: Iterable[NamespaceVisitor],
         print_output_handler: Callable[[int, str], None],
         allow_unregulated_fixed_port_id: bool,
     ):
         self._definition = definition
         self._lookup_definitions = list(lookup_definitions)
+        self._namespace_visitors = namespace_visitors
         self._print_output_handler = print_output_handler
         self._allow_unregulated_fixed_port_id = allow_unregulated_fixed_port_id
         self._element_callback = None  # type: Optional[Callable[[str], None]]
 
-        assert isinstance(self._definition, _dsdl_definition.DSDLDefinition)
-        assert all(map(lambda x: isinstance(x, _dsdl_definition.DSDLDefinition), lookup_definitions))
+        assert isinstance(self._definition, DsdlFileBuildable)
+        assert all(map(lambda x: isinstance(x, DsdlFileBuildable), lookup_definitions))
         assert callable(self._print_output_handler)
         assert isinstance(self._allow_unregulated_fixed_port_id, bool)
 
@@ -198,6 +228,9 @@ def resolve_versioned_data_type(self, name: str, version: _serializable.Version)
         del name
         found = list(filter(lambda d: d.full_name == full_name and d.version == version, self._lookup_definitions))
         if not found:
+            for visitor in self._namespace_visitors:
+                visitor.on_discover_lookup_dependent_type(self._definition, full_name, version)
+
             # Play Sherlock to help the user with mistakes like https://forum.opencyphal.org/t/904/2
             requested_ns = full_name.split(_serializable.CompositeType.NAME_COMPONENT_SEPARATOR)[0]
             lookup_nss = set(x.root_namespace for x in self._lookup_definitions)
@@ -221,15 +254,20 @@ def resolve_versioned_data_type(self, name: str, version: _serializable.Version)
             raise _error.InternalError("Conflicting definitions: %r" % found)
 
         target_definition = found[0]
-        assert isinstance(target_definition, _dsdl_definition.DSDLDefinition)
+        for visitor in self._namespace_visitors:
+            visitor.on_discover_lookup_dependent_file(self._definition, target_definition)
+
+        assert isinstance(target_definition, DsdlFileBuildable)
         assert target_definition.full_name == full_name
         assert target_definition.version == version
         # Recursion is cool.
-        return target_definition.read(
+        dt = target_definition.read(
             lookup_definitions=self._lookup_definitions,
+            namespace_visitors=self._namespace_visitors,
             print_output_handler=self._print_output_handler,
             allow_unregulated_fixed_port_id=self._allow_unregulated_fixed_port_id,
         )
+        return dt
 
     def _queue_attribute(self, element_callback: Callable[[str], None]) -> None:
         self._flush_attribute("")

pydsdl/_dsdl_definition.py

@@ -2,14 +2,16 @@
 # This software is distributed under the terms of the MIT License.
 # Author: Pavel Kirienko <pavel@opencyphal.org>
 
-import time
-from typing import Iterable, Callable, Optional, List
 import logging
+import time
 from pathlib import Path
-from ._error import FrontendError, InvalidDefinitionError, InternalError
-from ._serializable import CompositeType, Version
-from . import _parser
+from typing import Callable, Iterable, List, Optional
 
+from . import _parser
+from ._data_type_builder import DataTypeBuilder, DsdlFileBuildable
+from ._error import FrontendError, InternalError, InvalidDefinitionError
+from ._serializable import CompositeType, Version
+from .visitors import NamespaceVisitor
 
 _logger = logging.getLogger(__name__)
 
@@ -23,7 +25,7 @@ def __init__(self, text: str, path: Path):
         super().__init__(text=text, path=Path(path))
 
 
-class DSDLDefinition:
+class DSDLDefinition(DsdlFileBuildable):
     """
     A DSDL type definition source abstracts the filesystem level details away, presenting a higher-level
     interface that operates solely on the level of type names, namespaces, fixed identifiers, and so on.
@@ -86,26 +88,16 @@ def __init__(self, file_path: Path, root_namespace_path: Path):
 
         self._cached_type: Optional[CompositeType] = None
 
+    # +-----------------------------------------------------------------------+
+    # | DsdlFileBuildable :: INTERFACE                                        |
+    # +-----------------------------------------------------------------------+
     def read(
         self,
-        lookup_definitions: Iterable["DSDLDefinition"],
+        lookup_definitions: Iterable[DsdlFileBuildable],
+        namespace_visitors: Iterable[NamespaceVisitor],
         print_output_handler: Callable[[int, str], None],
         allow_unregulated_fixed_port_id: bool,
     ) -> CompositeType:
-        """
-        Reads the data type definition and returns its high-level data type representation.
-        The output is cached; all following invocations will read from the cache.
-        Caching is very important, because it is expected that the same definition may be referred to multiple
-        times (e.g., for composition or when accessing external constants). Re-processing a definition every time
-        it is accessed would be a huge waste of time.
-        Note, however, that this may lead to unexpected complications if one is attempting to re-read a definition
-        with different inputs (e.g., different lookup paths) expecting to get a different result: caching would
-        get in the way. That issue is easy to avoid by creating a new instance of the object.
-        :param lookup_definitions:              List of definitions available for referring to.
-        :param print_output_handler:            Used for @print and for diagnostics: (line_number, text) -> None.
-        :param allow_unregulated_fixed_port_id: Do not complain about fixed unregulated port IDs.
-        :return: The data type representation.
-        """
         log_prefix = "%s.%d.%d" % (self.full_name, self.version.major, self.version.minor)
         if self._cached_type is not None:
             _logger.debug("%s: Cache hit", log_prefix)
@@ -124,17 +116,17 @@ def read(
             ", ".join(set(sorted(map(lambda x: x.root_namespace, lookup_definitions)))),
         )
         try:
-            builder = _data_type_builder.DataTypeBuilder(
+            builder = DataTypeBuilder(
                 definition=self,
                 lookup_definitions=lookup_definitions,
+                namespace_visitors=namespace_visitors,
                 print_output_handler=print_output_handler,
                 allow_unregulated_fixed_port_id=allow_unregulated_fixed_port_id,
             )
-            with open(self.file_path) as f:
-                _parser.parse(f.read(), builder)
 
-            self._cached_type = builder.finalize()
+            _parser.parse(self._text, builder)
 
+            self._cached_type = builder.finalize()
             _logger.info(
                 "%s: Processed in %.0f ms; category: %s, fixed port ID: %s",
                 log_prefix,
@@ -151,34 +143,35 @@ def read(
         except Exception as ex:  # pragma: no cover
             raise InternalError(culprit=ex, path=self.file_path) from ex
 
+    # +-----------------------------------------------------------------------+
+    # | DsdlFile :: INTERFACE                                                 |
+    # +-----------------------------------------------------------------------+
+    @property
+    def composite_type(self) -> Optional[CompositeType]:
+        return self._cached_type
+
     @property
     def full_name(self) -> str:
-        """The full name, e.g., uavcan.node.Heartbeat"""
         return self._name
 
     @property
     def name_components(self) -> List[str]:
-        """Components of the full name as a list, e.g., ['uavcan', 'node', 'Heartbeat']"""
         return self._name.split(CompositeType.NAME_COMPONENT_SEPARATOR)
 
     @property
     def short_name(self) -> str:
-        """The last component of the full name, e.g., Heartbeat of uavcan.node.Heartbeat"""
         return self.name_components[-1]
 
     @property
     def full_namespace(self) -> str:
-        """The full name without the short name, e.g., uavcan.node for uavcan.node.Heartbeat"""
         return str(CompositeType.NAME_COMPONENT_SEPARATOR.join(self.name_components[:-1]))
 
     @property
     def root_namespace(self) -> str:
-        """The first component of the full name, e.g., uavcan of uavcan.node.Heartbeat"""
         return self.name_components[0]
 
     @property
     def text(self) -> str:
-        """The source text in its raw unprocessed form (with comments, formatting intact, and everything)"""
         return self._text
 
     @property
@@ -187,7 +180,6 @@ def version(self) -> Version:
 
     @property
     def fixed_port_id(self) -> Optional[int]:
-        """Either the fixed port ID as integer, or None if not defined for this type."""
         return self._fixed_port_id
 
     @property
@@ -202,6 +194,10 @@ def file_path(self) -> Path:
     def root_namespace_path(self) -> Path:
         return self._root_namespace_path
 
+    # +-----------------------------------------------------------------------+
+    # | Python :: SPECIAL FUNCTIONS                                           |
+    # +-----------------------------------------------------------------------+
+
     def __eq__(self, other: object) -> bool:
         """
         Two definitions will compare equal if they share the same name AND version number.
@@ -220,8 +216,3 @@ def __str__(self) -> str:
         )
 
     __repr__ = __str__
-
-
-# Moved this import here to break recursive dependency.
-# Maybe I have messed up the architecture? Should think about it later.
-from . import _data_type_builder  # pylint: disable=wrong-import-position