Repository: googlefonts/ufo2ft
Pull request #466 (2021-02-18 22:18:02)

[filters] Add loadFilterFromString()
This add the ufo2ft.filters.loadFilterFromString() function to be used in fontmake.
See:
- https://github.com/googlefonts/fontmake/issues/288#issuecomment-781453343
- https://github.com/googlefonts/fontbakery/issues/2961#issuecomment-740108673


BaseFilter only takes kwargs instead of args and kwargs. None of the ufo2ft filters use the args.

Edit: The args can be given with kwargs key `args`.


Patch:
diff --git a/Lib/ufo2ft/featureWriters/__init__.py b/Lib/ufo2ft/featureWriters/__init__.py
index 461225c..b085394 100644
--- a/Lib/ufo2ft/featureWriters/__init__.py
+++ b/Lib/ufo2ft/featureWriters/__init__.py
@@ -1,7 +1,8 @@
 import importlib
-import re
 from inspect import isclass
 
+from ufo2ft.util import _loadPluginFromString
+
 from .baseFeatureWriter import BaseFeatureWriter
 from .kernFeatureWriter import KernFeatureWriter
 from .markFeatureWriter import MarkFeatureWriter
@@ -95,25 +96,6 @@ def loadFeatureWriters(ufo, ignoreErrors=True):
     return writers
 
 
-# NOTE about the security risk involved in using eval: the function below is
-# meant to be used to parse string coming from the command-line, which is
-# inherently "trusted"; if that weren't the case, a potential attacker
-# could do worse things than segfaulting the Python interpreter...
-
-
-def _kwargsEval(s):
-    return eval(
-        "dict(%s)" % s, {"__builtins__": {"True": True, "False": False, "dict": dict}}
-    )
-
-
-_featureWriterSpecRE = re.compile(
-    r"(?:([\w\.]+)::)?"  # MODULE_NAME + '::'
-    r"(\w+)"  # CLASS_NAME [required]
-    r"(?:\((.*)\))?"  # (KWARGS)
-)
-
-
 def loadFeatureWriterFromString(spec):
     """Take a string specifying a feature writer class to load (either a
     built-in writer or one defined in an external, user-defined module),
@@ -142,20 +124,4 @@ def loadFeatureWriterFromString(spec):
     >>> loadFeatureWriterFromString("ufo2ft.featureWriters::KernFeatureWriter")
     <ufo2ft.featureWriters.kernFeatureWriter.KernFeatureWriter object at ...>
     """
-    spec = spec.strip()
-    m = _featureWriterSpecRE.match(spec)
-    if not m or (m.end() - m.start()) != len(spec):
-        raise ValueError(spec)
-    moduleName = m.group(1) or "ufo2ft.featureWriters"
-    className = m.group(2)
-    kwargs = m.group(3)
-
-    module = importlib.import_module(moduleName)
-    klass = getattr(module, className)
-    if not isValidFeatureWriter(klass):
-        raise TypeError(klass)
-    try:
-        options = _kwargsEval(kwargs) if kwargs else {}
-    except SyntaxError:
-        raise ValueError("options have incorrect format: %r" % kwargs)
-    return klass(**options)
+    return _loadPluginFromString(spec, "ufo2ft.featureWriters", isValidFeatureWriter)
diff --git a/Lib/ufo2ft/filters/__init__.py b/Lib/ufo2ft/filters/__init__.py
index e29c566..cf78095 100644
--- a/Lib/ufo2ft/filters/__init__.py
+++ b/Lib/ufo2ft/filters/__init__.py
@@ -1,11 +1,36 @@
 import importlib
 import logging
-from types import SimpleNamespace
-
-from fontTools.misc.loggingTools import Timer
+from inspect import getfullargspec, isclass
 
 from ufo2ft.constants import FILTERS_KEY as UFO2FT_FILTERS_KEY  # keep previous name
-from ufo2ft.util import _GlyphSet, _LazyFontName
+from ufo2ft.util import _loadPluginFromString
+
+from .base import BaseFilter
+from .cubicToQuadratic import CubicToQuadraticFilter
+from .decomposeComponents import DecomposeComponentsFilter
+from .decomposeTransformedComponents import DecomposeTransformedComponentsFilter
+from .explodeColorLayerGlyphs import ExplodeColorLayerGlyphsFilter
+from .flattenComponents import FlattenComponentsFilter
+from .propagateAnchors import PropagateAnchorsFilter
+from .removeOverlaps import RemoveOverlapsFilter
+from .sortContours import SortContoursFilter
+from .transformations import TransformationsFilter
+
+__all__ = [
+    "BaseFilter",
+    "CubicToQuadraticFilter",
+    "DecomposeComponentsFilter",
+    "DecomposeTransformedComponentsFilter",
+    "ExplodeColorLayerGlyphsFilter",
+    "FlattenComponentsFilter",
+    "PropagateAnchorsFilter",
+    "RemoveOverlapsFilter",
+    "SortContoursFilter",
+    "TransformationsFilter",
+    "loadFilters",
+    "loadFilterFromString",
+]
+
 
 logger = logging.getLogger(__name__)
 
@@ -53,176 +78,44 @@ def loadFilters(ufo):
     return preFilters, postFilters
 
 
-class BaseFilter:
-
-    # tuple of strings listing the names of required positional arguments
-    # which will be set as attributes of the filter instance
-    _args = ()
-
-    # dictionary containing the names of optional keyword arguments and
-    # their default values, which will be set as instance attributes
-    _kwargs = {}
-
-    def __init__(self, *args, **kwargs):
-        self.options = options = SimpleNamespace()
-
-        # process positional arguments
-        num_required = len(self._args)
-        num_args = len(args)
-        if num_args < num_required:
-            missing = [repr(a) for a in self._args[num_args:]]
-            num_missing = len(missing)
-            raise TypeError(
-                "missing {} required positional argument{}: {}".format(
-                    num_missing, "s" if num_missing > 1 else "", ", ".join(missing)
-                )
-            )
-        elif num_args > num_required:
-            extra = [repr(a) for a in args[num_required:]]
-            num_extra = len(extra)
-            raise TypeError(
-                "got {} unsupported positional argument{}: {}".format(
-                    num_extra, "s" if num_extra > 1 else "", ", ".join(extra)
-                )
-            )
-        for key, value in zip(self._args, args):
-            setattr(options, key, value)
-
-        # process optional keyword arguments
-        for key, default in self._kwargs.items():
-            setattr(options, key, kwargs.pop(key, default))
-
-        # process special include/exclude arguments
-        include = kwargs.pop("include", None)
-        exclude = kwargs.pop("exclude", None)
-        if include is not None and exclude is not None:
-            raise ValueError("'include' and 'exclude' arguments are mutually exclusive")
-        if callable(include):
-            # 'include' can be a function (e.g. lambda) that takes a
-            # glyph object and returns True/False based on some test
-            self.include = include
-            self._include_repr = lambda: repr(include)
-        elif include is not None:
-            # or it can be a list of glyph names to be included
-            included = set(include)
-            self.include = lambda g: g.name in included
-            self._include_repr = lambda: repr(include)
-        elif exclude is not None:
-            # alternatively one can provide a list of names to not include
-            excluded = set(exclude)
-            self.include = lambda g: g.name not in excluded
-            self._exclude_repr = lambda: repr(exclude)
-        else:
-            # by default, all glyphs are included
-            self.include = lambda g: True
-
-        # raise if any unsupported keyword arguments
-        if kwargs:
-            num_left = len(kwargs)
-            raise TypeError(
-                "got {}unsupported keyword argument{}: {}".format(
-                    "an " if num_left == 1 else "",
-                    "s" if len(kwargs) > 1 else "",
-                    ", ".join(f"'{k}'" for k in kwargs),
-                )
-            )
-
-        # run the filter's custom initialization code
-        self.start()
-
-    def __repr__(self):
-        items = []
-        if self._args:
-            items.append(
-                ", ".join(repr(getattr(self.options, arg)) for arg in self._args)
-            )
-        if self._kwargs:
-            items.append(
-                ", ".join(
-                    "{}={!r}".format(k, getattr(self.options, k))
-                    for k in sorted(self._kwargs)
-                )
-            )
-        if hasattr(self, "_include_repr"):
-            items.append(f"include={self._include_repr()}")
-        elif hasattr(self, "_exclude_repr"):
-            items.append(f"exclude={self._exclude_repr()}")
-        return "{}({})".format(type(self).__name__, ", ".join(items))
-
-    def start(self):
-        """Subclasses can perform here custom initialization code."""
-        pass
-
-    def set_context(self, font, glyphSet):
-        """Populate a `self.context` namespace, which is reset before each
-        new filter call.
-
-        Subclasses can override this to provide contextual information
-        which depends on other data in the font that is not available in
-        the glyphs objects currently being filtered, or set any other
-        temporary attributes.
-
-        The default implementation simply sets the current font and glyphSet,
-        and initializes an empty set that keeps track of the names of the
-        glyphs that were modified.
-
-        Returns the namespace instance.
-        """
-        self.context = SimpleNamespace(font=font, glyphSet=glyphSet)
-        self.context.modified = set()
-        return self.context
-
-    def filter(self, glyph):
-        """This is where the filter is applied to a single glyph.
-        Subclasses must override this method, and return True
-        when the glyph was modified.
-        """
-        raise NotImplementedError
-
-    @property
-    def name(self):
-        return self.__class__.__name__
-
-    def __call__(self, font, glyphSet=None):
-        """Run this filter on all the included glyphs.
-        Return the set of glyph names that were modified, if any.
-
-        If `glyphSet` (dict) argument is provided, run the filter on
-        the glyphs contained therein (which may be copies).
-        Otherwise, run the filter in-place on the font's default
-        glyph set.
-        """
-        fontName = _LazyFontName(font)
-        if glyphSet is not None and getattr(glyphSet, "name", None):
-            logger.info("Running %s on %s-%s", self.name, fontName, glyphSet.name)
-        else:
-            logger.info("Running %s on %s", self.name, fontName)
-
-        if glyphSet is None:
-            glyphSet = _GlyphSet.from_layer(font)
-
-        context = self.set_context(font, glyphSet)
-
-        filter_ = self.filter
-        include = self.include
-        modified = context.modified
-
-        with Timer() as t:
-            # we sort the glyph names to make loop deterministic
-            for glyphName in sorted(glyphSet.keys()):
-                if glyphName in modified:
-                    continue
-                glyph = glyphSet[glyphName]
-                if include(glyph) and filter_(glyph):
-                    modified.add(glyphName)
-
-        num = len(modified)
-        if num > 0:
-            logger.debug(
-                "Took %.3fs to run %s on %d glyph%s",
-                t,
-                self.name,
-                len(modified),
-                "" if num == 1 else "s",
-            )
-        return modified
+def isValidFilter(klass):
+    """Return True if 'klass' is a valid filter class.
+    A valid filter class is a class (of type 'type'), that has
+    a '__call__' (bound method), with the signature matching the same method
+    from the BaseFilter class:
+
+           def __call__(self, font, feaFile, compiler=None)
+    """
+    if not isclass(klass):
+        logger.error("{klass!r} is not a class")
+        return False
+    if not callable(klass):
+        logger.error("{klass!r} is not callable")
+        return False
+    if getfullargspec(klass.__call__).args != getfullargspec(BaseFilter.__call__).args:
+        logger.error("{klass!r} '__call__' method has incorrect signature", klass)
+        return False
+    return True
+
+
+def loadFilterFromString(spec):
+    """Take a string specifying a filter class to load (either a built-in
+    filter or one defined in an external, user-defined module), initialize it
+    with given options and return the filter object.
+
+    The string must conform to the following notation:
+    - an optional python module, followed by '::'
+    - a required class name; the class must have a method called 'filter'
+      with the same signature as the BaseFilter.
+    - an optional list of keyword-only arguments enclosed by parentheses
+
+    Raises ValueError if the string doesn't conform to this specification;
+    TypeError if imported name is not a filter class; and ImportError if the
+    user-defined module cannot be imported.
+
+    Examples:
+
+    >>> loadFilterFromString("ufo2ft.filters.removeOverlaps::RemoveOverlapsFilter")
+    <ufo2ft.filters.removeOverlaps.RemoveOverlapsFilter object at ...>
+    """
+    return _loadPluginFromString(spec, "ufo2ft.filters", isValidFilter)
diff --git a/Lib/ufo2ft/filters/base.py b/Lib/ufo2ft/filters/base.py
new file mode 100644
index 0000000..55847d2
--- /dev/null
+++ b/Lib/ufo2ft/filters/base.py
@@ -0,0 +1,200 @@
+import logging
+from types import SimpleNamespace
+
+from fontTools.misc.loggingTools import Timer
+
+from ufo2ft.util import _GlyphSet, _LazyFontName
+
+logger = logging.getLogger(__name__)
+
+
+class BaseFilter:
+
+    # tuple of strings listing the names of required positional arguments
+    # which will be set as attributes of the filter instance
+    _args = ()
+
+    # dictionary containing the names of optional keyword arguments and
+    # their default values, which will be set as instance attributes
+    _kwargs = {}
+
+    def __init__(self, *args, **kwargs):
+        self.options = options = SimpleNamespace()
+
+        num_required = len(self._args)
+        num_args = len(args)
+        # process positional arguments as keyword arguments
+        if num_args < num_required:
+            args = (
+                *args,
+                *(kwargs.pop(a) for a in self._args[num_args:] if a in kwargs),
+            )
+            num_args = len(args)
+            duplicated_args = [k for k in self._args if k in kwargs]
+            if duplicated_args:
+                num_duplicated = len(duplicated_args)
+                raise TypeError(
+                    "got {} duplicated positional argument{}: {}".format(
+                        num_duplicated,
+                        "s" if num_duplicated > 1 else "",
+                        ", ".join(duplicated_args),
+                    )
+                )
+        # process positional arguments
+        if num_args < num_required:
+            missing = [repr(a) for a in self._args[num_args:]]
+            num_missing = len(missing)
+            raise TypeError(
+                "missing {} required positional argument{}: {}".format(
+                    num_missing, "s" if num_missing > 1 else "", ", ".join(missing)
+                )
+            )
+        elif num_args > num_required:
+            extra = [repr(a) for a in args[num_required:]]
+            num_extra = len(extra)
+            raise TypeError(
+                "got {} unsupported positional argument{}: {}".format(
+                    num_extra, "s" if num_extra > 1 else "", ", ".join(extra)
+                )
+            )
+        for key, value in zip(self._args, args):
+            setattr(options, key, value)
+
+        # process optional keyword arguments
+        for key, default in self._kwargs.items():
+            setattr(options, key, kwargs.pop(key, default))
+
+        # process special include/exclude arguments
+        include = kwargs.pop("include", None)
+        exclude = kwargs.pop("exclude", None)
+        if include is not None and exclude is not None:
+            raise ValueError("'include' and 'exclude' arguments are mutually exclusive")
+        if callable(include):
+            # 'include' can be a function (e.g. lambda) that takes a
+            # glyph object and returns True/False based on some test
+            self.include = include
+            self._include_repr = lambda: repr(include)
+        elif include is not None:
+            # or it can be a list of glyph names to be included
+            included = set(include)
+            self.include = lambda g: g.name in included
+            self._include_repr = lambda: repr(include)
+        elif exclude is not None:
+            # alternatively one can provide a list of names to not include
+            excluded = set(exclude)
+            self.include = lambda g: g.name not in excluded
+            self._exclude_repr = lambda: repr(exclude)
+        else:
+            # by default, all glyphs are included
+            self.include = lambda g: True
+
+        # raise if any unsupported keyword arguments
+        if kwargs:
+            num_left = len(kwargs)
+            raise TypeError(
+                "got {}unsupported keyword argument{}: {}".format(
+                    "an " if num_left == 1 else "",
+                    "s" if len(kwargs) > 1 else "",
+                    ", ".join(f"'{k}'" for k in kwargs),
+                )
+            )
+
+        # run the filter's custom initialization code
+        self.start()
+
+    def __repr__(self):
+        items = []
+        if self._args:
+            items.append(
+                ", ".join(repr(getattr(self.options, arg)) for arg in self._args)
+            )
+        if self._kwargs:
+            items.append(
+                ", ".join(
+                    "{}={!r}".format(k, getattr(self.options, k))
+                    for k in sorted(self._kwargs)
+                )
+            )
+        if hasattr(self, "_include_repr"):
+            items.append(f"include={self._include_repr()}")
+        elif hasattr(self, "_exclude_repr"):
+            items.append(f"exclude={self._exclude_repr()}")
+        return "{}({})".format(type(self).__name__, ", ".join(items))
+
+    def start(self):
+        """Subclasses can perform here custom initialization code."""
+        pass
+
+    def set_context(self, font, glyphSet):
+        """Populate a `self.context` namespace, which is reset before each
+        new filter call.
+
+        Subclasses can override this to provide contextual information
+        which depends on other data in the font that is not available in
+        the glyphs objects currently being filtered, or set any other
+        temporary attributes.
+
+        The default implementation simply sets the current font and glyphSet,
+        and initializes an empty set that keeps track of the names of the
+        glyphs that were modified.
+
+        Returns the namespace instance.
+        """
+        self.context = SimpleNamespace(font=font, glyphSet=glyphSet)
+        self.context.modified = set()
+        return self.context
+
+    def filter(self, glyph):
+        """This is where the filter is applied to a single glyph.
+        Subclasses must override this method, and return True
+        when the glyph was modified.
+        """
+        raise NotImplementedError
+
+    @property
+    def name(self):
+        return self.__class__.__name__
+
+    def __call__(self, font, glyphSet=None):
+        """Run this filter on all the included glyphs.
+        Return the set of glyph names that were modified, if any.
+
+        If `glyphSet` (dict) argument is provided, run the filter on
+        the glyphs contained therein (which may be copies).
+        Otherwise, run the filter in-place on the font's default
+        glyph set.
+        """
+        fontName = _LazyFontName(font)
+        if glyphSet is not None and getattr(glyphSet, "name", None):
+            logger.info("Running %s on %s-%s", self.name, fontName, glyphSet.name)
+        else:
+            logger.info("Running %s on %s", self.name, fontName)
+
+        if glyphSet is None:
+            glyphSet = _GlyphSet.from_layer(font)
+
+        context = self.set_context(font, glyphSet)
+
+        filter_ = self.filter
+        include = self.include
+        modified = context.modified
+
+        with Timer() as t:
+            # we sort the glyph names to make loop deterministic
+            for glyphName in sorted(glyphSet.keys()):
+                if glyphName in modified:
+                    continue
+                glyph = glyphSet[glyphName]
+                if include(glyph) and filter_(glyph):
+                    modified.add(glyphName)
+
+        num = len(modified)
+        if num > 0:
+            logger.debug(
+                "Took %.3fs to run %s on %d glyph%s",
+                t,
+                self.name,
+                len(modified),
+                "" if num == 1 else "s",
+            )
+        return modified
diff --git a/Lib/ufo2ft/util.py b/Lib/ufo2ft/util.py
index 8a48d2a..210852f 100644
--- a/Lib/ufo2ft/util.py
+++ b/Lib/ufo2ft/util.py
@@ -1,4 +1,6 @@
+import importlib
 import logging
+import re
 from copy import deepcopy
 from inspect import getfullargspec
 
@@ -419,3 +421,43 @@ def _getDefaultNotdefGlyph(designSpaceDoc):
         except KeyError:
             notdefGlyph = None
     return notdefGlyph
+
+
+# NOTE about the security risk involved in using eval: the function below is
+# meant to be used to parse string coming from the command-line, which is
+# inherently "trusted"; if that weren't the case, a potential attacker
+# could do worse things than segfaulting the Python interpreter...
+
+
+def _kwargsEval(s):
+    return eval(
+        "dict(%s)" % s, {"__builtins__": {"True": True, "False": False, "dict": dict}}
+    )
+
+
+_pluginSpecRE = re.compile(
+    r"(?:([\w\.]+)::)?"  # MODULE_NAME + '::'
+    r"(\w+)"  # CLASS_NAME [required]
+    r"(?:\((.*)\))?"  # (KWARGS)
+)
+
+
+def _loadPluginFromString(spec, moduleName, isValidFunc):
+    spec = spec.strip()
+    m = _pluginSpecRE.match(spec)
+    if not m or (m.end() - m.start()) != len(spec):
+        raise ValueError(spec)
+    moduleName = m.group(1) or moduleName
+    className = m.group(2)
+    kwargs = m.group(3)
+
+    module = importlib.import_module(moduleName)
+    klass = getattr(module, className)
+    if not isValidFunc(klass):
+        raise TypeError(klass)
+    try:
+        options = _kwargsEval(kwargs) if kwargs else {}
+    except SyntaxError:
+        raise ValueError("options have incorrect format: %r" % kwargs)
+
+    return klass(**options)


Test patch:
diff --git a/tests/filters/filters_test.py b/tests/filters/filters_test.py
index f2a68b0..385878b 100644
--- a/tests/filters/filters_test.py
+++ b/tests/filters/filters_test.py
@@ -7,6 +7,7 @@ from ufo2ft.filters import (
     UFO2FT_FILTERS_KEY,
     BaseFilter,
     getFilterClass,
+    loadFilterFromString,
     loadFilters,
     logger,
 )
@@ -122,6 +123,26 @@ def test_loadFilters_args_unsupported(ufo):
     assert exc_info.match("unsupported")
 
 
+def test_loadFilters_args_as_keywords(ufo):
+    del ufo.lib[UFO2FT_FILTERS_KEY][0]["args"]
+    ufo.lib[UFO2FT_FILTERS_KEY][0]["kwargs"] = {"a": "foo", "b": "bar"}
+
+    _, [filter_obj] = loadFilters(ufo)
+
+    assert filter_obj.options.a == "foo"
+    assert filter_obj.options.b == "bar"
+
+
+def test_loadFilters_args_as_duplicated_keywords(ufo):
+    ufo.lib[UFO2FT_FILTERS_KEY][0]["args"] = ["foo"]
+    ufo.lib[UFO2FT_FILTERS_KEY][0]["kwargs"] = {"a": "foo", "b": "bar"}
+
+    with pytest.raises(TypeError) as exc_info:
+        loadFilters(ufo)
+
+    assert exc_info.match("duplicated")
+
+
 def test_loadFilters_include_all(ufo):
     _, [filter_obj] = loadFilters(ufo)
 
@@ -179,6 +200,33 @@ def test_loadFilters_kwargs_unsupported(ufo):
     assert exc_info.match("got an unsupported keyword")
 
 
+VALID_SPEC_STRINGS = [
+    "RemoveOverlapsFilter",
+    "PropagateAnchorsFilter(include=['a', 'b', 'c'])",
+    "ufo2ft.filters.fooBar::FooBarFilter(a='a', b='b', c=1)",
+]
+
+
+@pytest.mark.parametrize("spec", VALID_SPEC_STRINGS)
+def test_loadFilterFromString(spec, ufo):
+    philter = loadFilterFromString(spec)
+    assert callable(philter)
+
+
+def test_loadFilterFromString_args_missing(ufo):
+    with pytest.raises(TypeError) as info:
+        loadFilterFromString(
+            "ufo2ft.filters.fooBar::FooBarFilter(a='a', c=1)",
+        )
+    assert info.match("missing 1 required positional argument: 'b'")
+
+    with pytest.raises(TypeError) as info:
+        loadFilterFromString(
+            "ufo2ft.filters.fooBar::FooBarFilter(c=1)",
+        )
+    assert info.match("missing 2 required positional arguments: 'a', 'b'")
+
+
 def test_BaseFilter_repr():
     class NoArgFilter(BaseFilter):
         pass