Additional strictness and some logging

DEBUG_template.toml

@@ -45,6 +45,7 @@
 # profiler_type = "cprofile"
 # profiler_type = "profile"
 # profiler_type = "pyinstrument"
+# profiler_type = "monkeytype"
 
 ### enable profiler, see cmd line arg -p, default: false
 # profile_enable = true

lib/commands.py

@@ -300,11 +300,11 @@ def _standalone_commands(svg):
             pass
 
 
-def is_command(node):
+def is_command(node: inkex.BaseElement) -> bool:
     return CONNECTION_START in node.attrib or CONNECTION_END in node.attrib
 
 
-def is_command_symbol(node):
+def is_command_symbol(node: inkex.BaseElement) -> bool:
     symbol = None
     xlink = node.get(XLINK_HREF, "")
     if xlink.startswith("#inkstitch_"):
@@ -313,23 +313,23 @@ def is_command_symbol(node):
 
 
 @cache
-def symbols_path():
+def symbols_path() -> str:
     return os.path.join(get_bundled_dir("symbols"), "inkstitch.svg")
 
 
 @cache
-def symbols_svg():
+def symbols_svg() -> inkex.BaseElement:
     with open(symbols_path()) as symbols_file:
         return inkex.load_svg(symbols_file).getroot()
 
 
 @cache
-def symbol_defs():
+def symbol_defs() -> inkex.BaseElement:
     return symbols_svg().defs
 
 
 @cache
-def ensure_symbol(svg, command):
+def ensure_symbol(svg, command) -> None:
     """Make sure the command's symbol definition exists in the <svg:defs> tag."""
 
     # using @cache really just makes sure that we don't bother ensuring the

lib/debug/utils.py

@@ -286,13 +286,20 @@ def with_pyinstrument(extension, remaining_args, profile_file_path: Path):
 
 def with_monkeytype(extension, remaining_args, profile_file_path: Path) -> None:
     '''
-    'profile' with monkeytype to get some class information
+    'profile' with monkeytype to get type information. This may be handy for anyone who wants to
+    add type annotations to older parts of our code that don't have them.
+
+    See https://monkeytype.readthedocs.io/en/stable/generation.html for usage instructions.
     '''
     import monkeytype  # type: ignore[import-untyped,import-not-found]
 
-    class CustomMonkeyConfig(monkeytype.config.DefaultConfig):
-        def trace_store(self):
-            return monkeytype.db.sqlite.SQLiteStore.make_store(str(profile_file_path))
+    # Monkeytype will use these environment variables for the db path and to filter the modules respectively.
+    # This is easier than using monkeytype's actual config API, anyway.
+    os.environ["MT_DB_PATH"] = str(profile_file_path)
+    os.environ["MONKEYTYPE_TRACE_MODULES"] = str(Path(__file__).parents[2].name)
 
-    with monkeytype.trace(CustomMonkeyConfig()):
+    with monkeytype.trace():
         extension.run(args=remaining_args)
+
+    print(f"Profiler: monkeytype, db written to '{profile_file_path}'.\n\
+          Run 'MT_DB_PATH={profile_file_path} monkeytype ...' from the inkstitch repo directory.", file=sys.stderr)

lib/elements/clone.py

@@ -5,11 +5,11 @@
 
 from contextlib import contextmanager
 from math import degrees
-from typing import Dict, Generator, List, Optional
+from typing import Dict, Generator, List, Optional, Tuple, Any, override, cast
 
-from inkex import BaseElement, Title, Transform
+from inkex import BaseElement, Title, Transform, Vector2d
 from lxml.etree import _Comment
-from shapely import MultiLineString
+from shapely import Geometry, MultiLineString, Point as ShapelyPoint
 
 from ..commands import (find_commands, is_command_symbol,
                         point_command_symbols_up)
@@ -40,12 +40,13 @@ class Clone(EmbroideryElement):
     name = "Clone"
     element_name = _("Clone")
 
-    def __init__(self, *args, **kwargs):
-        super(Clone, self).__init__(*args, **kwargs)
+    @override
+    def __init__(self, node: BaseElement) -> None:
+        super(Clone, self).__init__(node)
 
     @property
     @param('clone', _("Clone"), type='toggle', inverse=False, default=True)
-    def clone(self):
+    def clone(self) -> bool:
         return self.get_boolean_param("clone", True)
 
     @property
@@ -55,7 +56,7 @@ class Clone(EmbroideryElement):
            unit='deg',
            type='float')
     @cache
-    def clone_fill_angle(self):
+    def clone_fill_angle(self) -> float:
         return self.get_float_param('angle')
 
     @property
@@ -66,15 +67,16 @@ class Clone(EmbroideryElement):
            type='boolean',
            default=False)
     @cache
-    def flip_angle(self):
+    def flip_angle(self) -> bool:
         return self.get_boolean_param('flip_angle', False)
 
-    def get_cache_key_data(self, previous_stitch, next_element):
+    @override
+    def get_cache_key_data(self, previous_stitch: Any, next_element: EmbroideryElement) -> List[str]:
         source_node = self.node.href
         source_elements = self.clone_to_elements(source_node)
         return [element.get_cache_key(previous_stitch, next_element) for element in source_elements]
 
-    def clone_to_elements(self, node) -> List[EmbroideryElement]:
+    def clone_to_elements(self, node: BaseElement) -> List[EmbroideryElement]:
         # Only used in get_cache_key_data, actual embroidery uses nodes_to_elements+iterate_nodes
         from .utils import node_to_elements
         elements = []
@@ -85,7 +87,8 @@ class Clone(EmbroideryElement):
                 elements.extend(node_to_elements(child, True))
         return elements
 
-    def to_stitch_groups(self, last_stitch_group=None, next_element=None) -> List[StitchGroup]:
+    @override
+    def to_stitch_groups(self, last_stitch_group: Optional[StitchGroup], next_element: Optional[EmbroideryElement] = None) -> List[StitchGroup]:
         if not self.clone:
             return []
 
@@ -96,7 +99,7 @@ class Clone(EmbroideryElement):
 
             next_elements = [next_element]
             if len(elements) > 1:
-                next_elements = elements[1:] + next_elements
+                next_elements = cast(List[Optional[EmbroideryElement]], elements[1:]) + next_elements
             for element, next_element in zip(elements, next_elements):
                 # Using `embroider` here to get trim/stop after commands, etc.
                 element_stitch_groups = element.embroider(last_stitch_group, next_element)
@@ -107,26 +110,28 @@ class Clone(EmbroideryElement):
             return stitch_groups
 
     @property
-    def first_stitch(self):
+    def first_stitch(self) -> Optional[ShapelyPoint]:
         first, last = self.first_and_last_element()
         if first:
             return first.first_stitch
         return None
 
-    def uses_previous_stitch(self):
+    @override
+    def uses_previous_stitch(self) -> bool:
         first, last = self.first_and_last_element()
         if first:
             return first.uses_previous_stitch()
-        return None
+        return False
 
-    def uses_next_element(self):
+    @override
+    def uses_next_element(self) -> bool:
         first, last = self.first_and_last_element()
         if last:
             return last.uses_next_element()
-        return None
+        return False
 
     @cache
-    def first_and_last_element(self):
+    def first_and_last_element(self) -> Tuple[Optional[EmbroideryElement], Optional[EmbroideryElement]]:
         with self.clone_elements() as elements:
             if len(elements):
                 return elements[0], elements[-1]
@@ -153,7 +158,7 @@ class Clone(EmbroideryElement):
             for cloned_node in cloned_nodes:
                 cloned_node.delete()
 
-    def resolve_clone(self, recursive=True) -> List[BaseElement]:
+    def resolve_clone(self, recursive: bool = True) -> List[BaseElement]:
         """
         "Resolve" this clone element by copying the node it hrefs as if unlinking the clone in Inkscape.
         The node will be added as a sibling of this element's node, with its transform and style applied.
@@ -255,14 +260,14 @@ class Clone(EmbroideryElement):
             node.set(INKSTITCH_ATTRIBS['angle'], round(element_angle, 6))
 
     @property
-    def shape(self):
+    def shape(self) -> Geometry:
         path = self.node.get_path()
         transform = Transform(self.node.composed_transform())
         path = path.transform(transform)
         path = path.to_superpath()
         return MultiLineString(path[0])
 
-    def center(self, source_node):
+    def center(self, source_node: BaseElement) -> Vector2d:
         translate = Transform(f"translate({float(self.node.get('x', '0'))}, {float(self.node.get('y', '0'))})")
         parent = self.node.getparent()
         assert parent is not None, "This should be part of a tree and therefore have a parent"
@@ -270,13 +275,13 @@ class Clone(EmbroideryElement):
         center = self.node.bounding_box(transform).center
         return center
 
-    def validation_warnings(self):
+    def validation_warnings(self) -> Generator[CloneWarning, Any, None]:
         source_node = self.node.href
         point = self.center(source_node)
         yield CloneWarning(point)
 
 
-def is_clone(node):
+def is_clone(node: BaseElement) -> bool:
     if node.tag == SVG_USE_TAG and node.href is not None and not is_command_symbol(node):
         return True
     return False
@@ -304,7 +309,7 @@ def clone_with_fixup(parent: BaseElement, node: BaseElement) -> BaseElement:
 
     ret = clone_children(parent, node)
 
-    def fixup_id_attr(node: BaseElement, attr: str):
+    def fixup_id_attr(node: BaseElement, attr: str) -> None:
         # Replace the id value for this attrib with the corresponding one in the clone subtree, if applicable.
         val = node.get(attr)
         if val is not None:

lib/elements/element.py

@@ -2,6 +2,7 @@
 #
 # Copyright (c) 2010 Authors
 # Licensed under the GNU GPL version 3.0 or later.  See the file LICENSE for details.
+from __future__ import annotations
 import json
 import sys
 from contextlib import contextmanager
@@ -456,7 +457,7 @@ class EmbroideryElement(object):
         raise NotImplementedError("INTERNAL ERROR: %s must implement shape()", self.__class__)
 
     @property
-    def first_stitch(self):
+    def first_stitch(self) -> Optional[ShapelyPoint]:
         # first stitch is an approximation to where the first stitch may possibly be
         # if not defined through commands or repositioned by the previous element
         raise NotImplementedError("INTERNAL ERROR: %s must implement first_stitch()", self.__class__)
@@ -522,7 +523,7 @@ class EmbroideryElement(object):
 
         return lock_start, lock_end
 
-    def to_stitch_groups(self, last_stitch_group: Optional[StitchGroup], next_element: Optional[ShapelyPoint] = None) -> List[StitchGroup]:
+    def to_stitch_groups(self, last_stitch_group: Optional[StitchGroup], next_element: Optional[EmbroideryElement] = None) -> List[StitchGroup]:
         raise NotImplementedError("%s must implement to_stitch_groups()" % self.__class__.__name__)
 
     @debug.time

lib/elements/satin_column.py

@@ -747,7 +747,7 @@ class SatinColumn(EmbroideryElement):
 
         rails = list(self.flattened_rails)
         rungs = self.flattened_rungs
-        cut_points: typing.List[typing.List[float]] = [[], []]
+        cut_points = [[], []]
         for rung in rungs:
             intersections = rung.intersection(shgeo.MultiLineString(rails))
             # ignore the rungs that are cutting a rail multiple times
@@ -961,7 +961,7 @@ class SatinColumn(EmbroideryElement):
 
         rails = [shgeo.LineString(self.flatten_subpath(rail)) for rail in self.rails]
 
-        path_lists: typing.List[typing.List[shgeo.LineString]] = [[], []]  # List of list of linestrings
+        path_lists = [[], []]
 
         rails_to_reverse = self._get_rails_to_reverse()
 

lib/elements/utils.py

@@ -3,7 +3,7 @@
 # Copyright (c) 2010 Authors
 # Licensed under the GNU GPL version 3.0 or later.  See the file LICENSE for details.
 
-from typing import List, Optional
+from typing import List, Optional, Iterable
 
 from inkex import BaseElement
 from lxml.etree import Comment
@@ -73,7 +73,7 @@ def node_to_elements(node, clone_to_element=False) -> List[EmbroideryElement]:
         return []
 
 
-def nodes_to_elements(nodes):
+def nodes_to_elements(nodes: Iterable[BaseElement]) -> List[EmbroideryElement]:
     elements = []
     for node in nodes:
         elements.extend(node_to_elements(node))
@@ -89,7 +89,8 @@ def iterate_nodes(node: BaseElement,  # noqa: C901
     def walk(node: BaseElement, selected: bool) -> List[BaseElement]:
         nodes = []
 
-        if node.tag == Comment:
+        # lxml-stubs types are wrong, node.tag can be Comment.
+        if node.tag is Comment:  # type:ignore[comparison-overlap]
             return []
 
         element = EmbroideryElement(node)

mypy.ini

@@ -8,12 +8,21 @@ allow_redefinition = True
 # Ignore that for now.
 disable_error_code = misc
 
+# A handful of strictness increases we can pass as it is
 warn_unused_configs = True
 warn_redundant_casts = True
 warn_unused_ignores = True
 
-[mypy-lib.elements.*]
+strict_equality = True
+extra_checks = True
+disallow_subclassing_any = True
+disallow_untyped_decorators = True
+
+# An example of increased strictness: We can increase the strictness of parts of the code as we go.
+[mypy-lib.elements.clone]
 check_untyped_defs = True
+disallow_incomplete_defs = True
+disallow_untyped_defs = True
 
 # This part of the code will need some work before it'll start passing.
 [mypy-lib.tartan.*]