Refactor EastAsianSpacingTester

* Add more type annotations.
* Prepare for "non-fullwidth + full-width" combinations.

east_asian_spacing/builder.py

@@ -182,14 +182,11 @@ def save_glyphs(self, output, **kwargs):
         united_spacing = self._united_spacings()
         united_spacing.save_glyphs(output, **kwargs)
 
-    def _testers(self):
+    def _testers(self, config: Config):
         for spacing in self._spacings:
             assert len(spacing.changed_fonts) > 0
             for font in spacing.changed_fonts:
-                tester = EastAsianSpacingTester(
-                    font,
-                    glyphs=spacing.horizontal.glyph_id_set,
-                    vertical_glyphs=spacing.vertical.glyph_id_set)
+                tester = EastAsianSpacingTester(font, config, spacing=spacing)
                 yield tester
 
     async def test(self, config=None, smoke=None):
@@ -200,8 +197,8 @@ async def test(self, config=None, smoke=None):
         elif smoke:
             config.for_smoke_testing()
         assert self.has_spacings
-        testers = self._testers()
-        coros = (tester.test(config) for tester in testers)
+        testers = self._testers(config)
+        coros = (tester.test() for tester in testers)
         await EastAsianSpacingTester.run_coros(coros, parallel=True)
 
     @classmethod

east_asian_spacing/shaper.py

@@ -18,6 +18,7 @@
 from typing import Optional
 from typing import Union
 from typing import Tuple
+from typing import Set
 
 import uharfbuzz as hb
 
@@ -187,7 +188,7 @@ def glyph_ids(self):
         return (g.glyph_id for g in self._glyphs)
 
     @property
-    def glyph_id_set(self):
+    def glyph_id_set(self) -> Set[int]:
         return set(self.glyph_ids)
 
     def isdisjoint(self, other: 'GlyphDataList'):

east_asian_spacing/tester.py

@@ -4,10 +4,16 @@
 import itertools
 import logging
 import math
+from typing import Iterable
+from typing import Optional
+from typing import Tuple
+from typing import Set
 
 from east_asian_spacing.config import Config
 from east_asian_spacing.font import Font
 from east_asian_spacing.shaper import Shaper
+from east_asian_spacing.spacing import EastAsianSpacing
+from east_asian_spacing.spacing import GlyphSets
 
 logger = logging.getLogger('test')
 
@@ -17,13 +23,14 @@ class ShapeTest(object):
     _vertical_features = (('vchw', 'fwid', 'vert'), ('fwid', 'vert'))
 
     @staticmethod
-    def create_list(font, inputs):
+    def create_list(font: Font, inputs: Iterable[Tuple[int, int]]):
         features = (ShapeTest._vertical_features
                     if font.is_vertical else ShapeTest._features)
         tests = tuple(ShapeTest(font, input, *features) for input in inputs)
         return tests
 
-    def __init__(self, font, input, features, off_features):
+    def __init__(self, font: Font, input: Tuple[int, int], features,
+                 off_features):
         self.font = font
         self.input = input
         self.features = features
@@ -43,15 +50,24 @@ async def shape(self, language):
         shaper.features = self.features
         self.glyphs = await shaper.shape(text)
 
-    def should_apply(self, em=None, glyphs=None):
+    def should_apply(self,
+                     index: int,
+                     glyph_id_sets: Optional[Tuple[Set[int]]],
+                     em=None):
         # If any glyphs are missing, or their advances are not em,
         # the feature should not apply.
         if em is None:
             em = self.font.fullwidth_advance
-        if any(g.glyph_id == 0 or g.advance != em for g in self.off_glyphs):
+        # Should not apply if any glyphs are missing.
+        if any(g.glyph_id == 0 for g in self.off_glyphs):
             return False
-        if glyphs is not None:
-            if not all(g.glyph_id in glyphs for g in self.off_glyphs):
+        # Should not apply if the advance of the target glyph is not 1em.
+        if self.off_glyphs[index].advance != em:
+            return False
+        if glyph_id_sets:
+            if self.off_glyphs[0].glyph_id not in glyph_id_sets[0]:
+                return False
+            if self.off_glyphs[1].glyph_id not in glyph_id_sets[1]:
                 return False
         return True
 
@@ -75,27 +91,33 @@ def __str__(self):
 
 class EastAsianSpacingTester(object):
 
-    def __init__(self, font, glyphs=None, vertical_glyphs=None):
+    def __init__(self,
+                 font: Font,
+                 config: Config,
+                 spacing: Optional[EastAsianSpacing] = None):
         self.font = font
-        self._glyphs = glyphs
-        self._vertical_glyphs = vertical_glyphs
+        self._config = config.for_font(font)
+        self._spacing = spacing
 
-    def _glyphs_for(self, font):
-        if font.is_vertical:
-            return self._vertical_glyphs or self._glyphs
-        return self._glyphs
-
-    async def test(self, config, fonts=None):
+    @property
+    def _glyph_sets(self) -> Optional[GlyphSets]:
+        if self._spacing:
+            if self.font.is_vertical:
+                return self._spacing.vertical
+            return self._spacing.horizontal
+        return None
+
+    async def test(self, fonts=None):
         fonts = fonts if fonts else (self.font, )
         fonts = itertools.chain(*(f.self_and_derived_fonts() for f in fonts))
         fonts = filter(lambda font: not font.is_collection, fonts)
         testers = tuple(
-            EastAsianSpacingTester(font, glyphs=self._glyphs_for(font))
+            EastAsianSpacingTester(font, self._config, spacing=self._spacing)
             for font in fonts)
         assert all(t.font == self.font
                    or t.font.root_or_self == self.font.root_or_self
                    for t in testers)
-        coros = (tester._test(config) for tester in testers)
+        coros = (tester._test() for tester in testers)
         # Run tests without using `asyncio.gather`
         # to avoid too many open files when using subprocesses.
         results = await EastAsianSpacingTester.run_coros(coros, parallel=False)
@@ -118,21 +140,31 @@ async def test(self, config, fonts=None):
                 '\n  '.join(summaries))
         logger.info('All %d fonts paased.', len(testers))
 
-    async def _test(self, config):
+    async def _test(self):
         coros = []
-        font = self.font
-        config = config.for_font(font)
+        config = self._config
         if not config:
             return tuple()
 
+        font = self.font
         opening = config.cjk_opening
         closing = config.cjk_closing
+        glyph_sets = self._glyph_sets
+        cl_op_tests = ShapeTest.create_list(
+            font, itertools.product(closing, opening))
         coros.append(
-            self.assert_trim(config, itertools.product(closing, opening), 0,
-                             False))
+            self.assert_trim(
+                cl_op_tests, 0, False,
+                (glyph_sets.left.glyph_id_set,
+                 glyph_sets.right.glyph_id_set) if glyph_sets else None))
+
+        op_op_tests = ShapeTest.create_list(
+            font, itertools.product(opening, opening))
         coros.append(
-            self.assert_trim(config, itertools.product(opening, opening), 1,
-                             True))
+            self.assert_trim(
+                op_op_tests, 1, True,
+                (glyph_sets.right.glyph_id_set,
+                 glyph_sets.right.glyph_id_set) if glyph_sets else None))
 
         # Run tests without using `asyncio.gather`
         # to avoid too many open files when using subprocesses.
@@ -141,9 +173,11 @@ async def _test(self, config):
         tests = tuple(itertools.chain(*tests))
         return tests
 
-    async def assert_trim(self, config, inputs, index, assert_offset):
+    async def assert_trim(self, tests: Iterable[ShapeTest], index: int,
+                          assert_offset: bool,
+                          glyph_id_sets: Optional[Tuple[Set[int]]]):
         font = self.font
-        tests = ShapeTest.create_list(font, inputs)
+        config = self._config
         coros = (test.shape(language=config.language) for test in tests)
         await EastAsianSpacingTester.run_coros(coros)
 
@@ -152,11 +186,13 @@ async def assert_trim(self, config, inputs, index, assert_offset):
         offset = em - half_em
         tested = []
         for test in tests:
-            if not test.should_apply(em=em, glyphs=self._glyphs):
+            if not test.should_apply(index, glyph_id_sets, em=em):
                 if test.glyphs != test.off_glyphs:
                     test.fail('Unexpected differences')
                     tested.append(test)
                 continue
+            assert test.glyphs
+            assert test.off_glyphs
             if test.glyphs[index].advance != half_em:
                 test.fail(f'{index}.advance != {half_em}')
             if (assert_offset and test.glyphs[index].offset -
@@ -193,7 +229,7 @@ async def main():
         if args.index >= 0:
             font = font.fonts_in_collection[args.index]
         config = Config.default
-        await EastAsianSpacingTester(font).test(config)
+        await EastAsianSpacingTester(font, config).test()
         logging.info('All tests pass')
 
 

tests/config_test.py

@@ -27,7 +27,7 @@ def __init__(self):
     fail_config = config.clone()
     fail_config.cjk_opening = {0x300A}
     with pytest.raises(AssertionError):
-        await EastAsianSpacingTester(builder.font).test(fail_config)
+        await EastAsianSpacingTester(builder.font, fail_config).test()
 
 
 def test_change_quotes_closing_to_opening():