gtristan commented on code in PR #1890:
URL: https://github.com/apache/buildstream/pull/1890#discussion_r1510855796


##########
src/buildstream/sourcemirror.py:
##########
@@ -0,0 +1,203 @@
+#
+#  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.
+#
+#  Authors:
+#        Tristan Van Berkom <[email protected]>
+"""
+SourceMirror - Base source mirror class
+=======================================
+The SourceMirror plugin allows one to customize how
+:func:`Source.translate_url() <buildstream.source.Source.translate_url>` will
+behave when looking up mirrors, allowing some additional flexibility in the
+implementation of source mirrors.
+
+
+.. _core_source_mirror_abstract_methods:
+
+Abstract Methods
+----------------
+For loading and configuration purposes, SourceMirrors may optionally implement
+the :func:`Plugin base class Plugin.configure() method 
<buildstream.plugin.Plugin.configure>`
+in order to load any custom configuration in the `config` dictionary.
+
+The remaining :ref:`Plugin base class abstract methods 
<core_plugin_abstract_methods>` are
+not relevant to the SourceMirror plugin object and need not be implemented.
+
+Sources expose the following abstract methods. Unless explicitly mentioned,
+these methods are mandatory to implement.
+
+* :func:`SourceMirror.translate_url() 
<buildstream.source.SourceMirror.translate_url>`
+ 
+  Produce an appropriate URL for the given URL and alias.
+
+
+Class Reference
+---------------
+"""
+
+from typing import Optional, Dict, List, Any, TYPE_CHECKING
+
+from .node import MappingNode, SequenceNode
+from .plugin import Plugin
+from ._exceptions import BstError, LoadError
+from .exceptions import ErrorDomain, LoadErrorReason
+
+if TYPE_CHECKING:
+
+    # pylint: disable=cyclic-import
+    from ._context import Context
+    from ._project import Project
+
+    # pylint: enable=cyclic-import
+
+
+class SourceMirrorError(BstError):
+    """This exception should be raised by :class:`.SourceMirror` 
implementations
+    to report errors to the user.
+
+    Args:
+       message: The breif error description to report to the user
+       detail: A possibly multiline, more detailed error message
+       reason: An optional machine readable reason string, used for test cases
+
+    *Since: 2.2*
+    """
+
+    def __init__(
+        self, message: str, *, detail: Optional[str] = None, reason: 
Optional[str] = None, temporary: bool = False
+    ):
+        super().__init__(message, detail=detail, domain=ErrorDomain.SOURCE, 
reason=reason)
+
+
+class SourceMirror(Plugin):
+    """SourceMirror()
+
+    Base SourceMirror class.
+
+    All SourceMirror plugins derive from this class, this interface defines how
+    the core will be interacting with SourceMirror plugins.
+
+    *Since: 2.2*
+    """
+
+    # The SourceMirror plugin type is only supported since BuildStream 2.2
+    BST_MIN_VERSION = "2.2"
+
+    def __init__(
+        self,
+        context: "Context",
+        project: "Project",
+        node: MappingNode,
+    ):
+        # Note: the MappingNode passed here is already expanded with
+        #       the project level base variables, so there is no need
+        #       to expand them redundantly here.
+        #
+        node.validate_keys(["name", "kind", "config", "aliases"])
+
+        # Do local base class parsing first
+        name: str = node.get_str("name")
+        self.__aliases: Dict[str, List[str]] = self.__load_aliases(node)
+
+        # Chain up to Plugin
+        super().__init__(name, context, project, node, "source-mirror")
+
+        # Plugin specific parsing
+        config = node.get_mapping("config", default={})
+        self._configure(config)
+
+    ##########################################################
+    #                        Public API                      #
+    ##########################################################
+    def translate_url(
+        self,
+        *,
+        project_name: str,
+        alias: str,
+        alias_url: str,

Review Comment:
   I actually spent some time inquiring on this.
   
   Having optional keyword parameters does not work with normal python, unless 
the plugin author uses `**kwargs` syntax to declare the function, this won't be 
very pretty and have nice type annotations. 
   
   If we instruct the plugin author to _only_ ever declare the function with 
`**kwargs` rather than explicitly typing out the parameters, then we can have 
functions with extensible keyword arguments in the API without breaking API.
   
   Alternatively, we can have some internal monstrosity which observes the 
python function in memory only only feeds the function with parameters it is 
prepared to accept, which might look more pretty in documentation but wont have 
any sensible type checking with mypy either.
   
   Keyword arguments are nice and extensible for us when we _provide_ a 
function that plugins _may call_, but not the other way around, we could go 
ahead with instructing plugin authors to use a `def function(self, **kwargs)` 
only, if we want extensibility, I'm not opposed to that but it does deviate 
from our current API style.
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to