bug 1385234, add reference entity list to checkers after creation, r=jotes
authorAxel Hecht <axel@pike.org>
Fri, 28 Jul 2017 13:14:37 +0200
changeset 297 c36e2fd09e8282431a933afed44969fbc09bb418
parent 294 e1c2731fa339b38dc4a937f86bc1ea88f3357fd0
child 298 1ca8623ce1563cb1c0b8fa405e9da148190baeb7
push id88
push useraxel@mozilla.com
push dateFri, 28 Jul 2017 15:34:52 +0000
reviewersjotes
bugs1385234
bug 1385234, add reference entity list to checkers after creation, r=jotes Switch the API on getChecker to allow the reference entity list to be avoided. Instead add a flag on the Checker class that asks for it to be set. This allows external users to inspect this, and use compare-locales checks more efficiently. MozReview-Commit-ID: 6oTbfJxVKBo
compare_locales/checks.py
compare_locales/compare.py
compare_locales/tests/test_checks.py
--- a/compare_locales/checks.py
+++ b/compare_locales/checks.py
@@ -13,36 +13,45 @@ except ImportError:
 
 from compare_locales.parser import DTDParser, PropertiesEntity
 
 
 class Checker(object):
     '''Abstract class to implement checks per file type.
     '''
     pattern = None
+    # if a check uses all reference entities, set this to True
+    needs_reference = False
 
     @classmethod
     def use(cls, file):
         return cls.pattern.match(file.file)
 
     def __init__(self, extra_tests):
         self.extra_tests = extra_tests
+        self.reference = None
 
     def check(self, refEnt, l10nEnt):
         '''Given the reference and localized Entities, performs checks.
 
         This is a generator yielding tuples of
         - "warning" or "error", depending on what should be reported,
         - tuple of line, column info for the error within the string
         - description string to be shown in the report
         '''
         if True:
             raise NotImplementedError("Need to subclass")
         yield ("error", (0, 0), "This is an example error", "example")
 
+    def set_reference(self, reference):
+        '''Set the reference entities.
+        Only do this if self.needs_reference is True.
+        '''
+        self.reference = reference
+
 
 class PrintfException(Exception):
     def __init__(self, msg, pos):
         self.pos = pos
         self.msg = msg
 
 
 class PropertiesChecker(Checker):
@@ -183,29 +192,29 @@ class DTDChecker(Checker):
 
     The code tries to parse until it doesn't find any unresolved entities
     anymore. If it finds one, it tries to grab the key, and adds an empty
     <!ENTITY key ""> definition to the header.
 
     Also checks for some CSS and number heuristics in the values.
     """
     pattern = re.compile('.*\.dtd$')
+    needs_reference = True  # to cast a wider net for known entity references
 
     eref = re.compile('&(%s);' % DTDParser.Name)
     tmpl = '''<!DOCTYPE elem [%s]>
 <elem>%s</elem>
 '''
     xmllist = set(('amp', 'lt', 'gt', 'apos', 'quot'))
 
-    def __init__(self, extra_tests, reference):
+    def __init__(self, extra_tests):
         super(DTDChecker, self).__init__(extra_tests)
         self.processContent = False
         if self.extra_tests is not None and 'android-dtd' in self.extra_tests:
             self.processContent = True
-        self.reference = reference
         self.__known_entities = None
 
     def known_entities(self, refValue):
         if self.__known_entities is None and self.reference is not None:
             self.__known_entities = set()
             for ent in self.reference:
                 self.__known_entities.update(self.entities_for_value(ent.val))
         return self.__known_entities if self.__known_entities is not None \
@@ -475,16 +484,16 @@ class FluentChecker(Checker):
             for attr in l10n_entry.attributes
             if attr.id.name in obsolete_attr_names
         ]
         for attr in obsolete_attrs:
             yield ('error', attr.span.start,
                    'Obsolete attribute: ' + attr.id.name, 'fluent')
 
 
-def getChecker(file, reference=None, extra_tests=None):
+def getChecker(file, extra_tests=None):
     if PropertiesChecker.use(file):
         return PropertiesChecker(extra_tests)
     if DTDChecker.use(file):
-        return DTDChecker(extra_tests, reference)
+        return DTDChecker(extra_tests)
     if FluentChecker.use(file):
         return FluentChecker(extra_tests)
     return None
--- a/compare_locales/compare.py
+++ b/compare_locales/compare.py
@@ -463,18 +463,19 @@ class ContentComparer:
 
         ar = AddRemove()
         ar.set_left(e.key for e in ref_entities)
         ar.set_right(e.key for e in l10n_entities)
         report = missing = obsolete = changed = unchanged = keys = 0
         missing_w = changed_w = unchanged_w = 0  # word stats
         missings = []
         skips = []
-        checker = getChecker(l10n,
-                             reference=ref_entities, extra_tests=extra_tests)
+        checker = getChecker(l10n, extra_tests=extra_tests)
+        if checker and checker.needs_reference:
+            checker.set_reference(ref_entities)
         for msg in p.findDuplicates(ref_entities):
             self.notify('warning', l10n, msg)
         for msg in p.findDuplicates(l10n_entities):
             self.notify('error', l10n, msg)
         for action, entity_id in ar:
             if action == 'delete':
                 # missing entity
                 if isinstance(ref_entities[ref_map[entity_id]], parser.Junk):
--- a/compare_locales/tests/test_checks.py
+++ b/compare_locales/tests/test_checks.py
@@ -14,29 +14,25 @@ class BaseHelper(unittest.TestCase):
     file = None
     refContent = None
 
     def setUp(self):
         p = getParser(self.file.file)
         p.readContents(self.refContent)
         self.refList, self.refMap = p.parse()
 
-    def _test(self, content, refWarnOrErrors, with_ref_file=False):
+    def _test(self, content, refWarnOrErrors):
         p = getParser(self.file.file)
         p.readContents(content)
         l10n = [e for e in p]
         assert len(l10n) == 1
         l10n = l10n[0]
-        if with_ref_file:
-            kwargs = {
-                'reference': self.refList
-            }
-        else:
-            kwargs = {}
-        checker = getChecker(self.file, **kwargs)
+        checker = getChecker(self.file)
+        if checker.needs_reference:
+            checker.set_reference(self.refList)
         ref = self.refList[self.refMap[l10n.key]]
         found = tuple(checker.check(ref, l10n))
         self.assertEqual(found, refWarnOrErrors)
 
 
 class TestProperties(BaseHelper):
     file = File('foo.properties', 'foo.properties')
     refContent = '''some = value
@@ -179,57 +175,51 @@ class TestEntitiesInDTDs(BaseHelper):
     file = File('foo.dtd', 'foo.dtd')
     refContent = '''<!ENTITY short "This is &brandShortName;">
 <!ENTITY shorter "This is &brandShorterName;">
 <!ENTITY ent.start "Using &brandShorterName; start to">
 <!ENTITY ent.end " end">
 '''
 
     def testOK(self):
-        self._test('''<!ENTITY ent.start "Mit &brandShorterName;">''', tuple(),
-                   with_ref_file=True)
+        self._test('''<!ENTITY ent.start "Mit &brandShorterName;">''', tuple())
 
     def testMismatch(self):
         self._test('''<!ENTITY ent.start "Mit &brandShortName;">''',
                    (('warning', (0, 0),
                      'Entity brandShortName referenced, '
                      'but brandShorterName used in context',
-                     'xmlparse'),),
-                   with_ref_file=True)
+                     'xmlparse'),))
 
     def testAcross(self):
         self._test('''<!ENTITY ent.end "Mit &brandShorterName;">''',
-                   tuple(),
-                   with_ref_file=True)
+                   tuple())
 
     def testAcrossWithMismatch(self):
         '''If we could tell that ent.start and ent.end are one string,
         we should warn. Sadly, we can't, so this goes without warning.'''
         self._test('''<!ENTITY ent.end "Mit &brandShortName;">''',
-                   tuple(),
-                   with_ref_file=True)
+                   tuple())
 
     def testUnknownWithRef(self):
         self._test('''<!ENTITY ent.start "Mit &foopy;">''',
                    (('warning',
                      (0, 0),
                      'Referencing unknown entity `foopy` '
                      '(brandShorterName used in context, '
                      'brandShortName known)',
-                     'xmlparse'),),
-                   with_ref_file=True)
+                     'xmlparse'),))
 
     def testUnknown(self):
         self._test('''<!ENTITY ent.end "Mit &foopy;">''',
                    (('warning',
                      (0, 0),
                      'Referencing unknown entity `foopy`'
                      ' (brandShortName, brandShorterName known)',
-                     'xmlparse'),),
-                   with_ref_file=True)
+                     'xmlparse'),))
 
 
 class TestAndroid(unittest.TestCase):
     """Test Android checker
 
     Make sure we're hitting our extra rules only if
     we're passing in a DTD file in the embedding/android module.
     """
@@ -377,17 +367,18 @@ class TestAndroid(unittest.TestCase):
         self.assertEqual(tuple(checker.check(ref, l10n)),
                          ())
 
     def test_entities_across_dtd(self):
         f = File("browser/strings.dtd", "strings.dtd", "browser")
         p = getParser(f.file)
         p.readContents('<!ENTITY other "some &good.ref;">')
         ref = p.parse()
-        checker = getChecker(f, reference=ref[0])
+        checker = getChecker(f)
+        checker.set_reference(ref[0])
         # good string
         ref = self.getDTDEntity("plain string")
         l10n = self.getDTDEntity("plain localized string")
         self.assertEqual(tuple(checker.check(ref, l10n)),
                          ())
         # dtd warning
         ref = self.getDTDEntity("plain string")
         l10n = self.getDTDEntity("plain localized string &ref;")