Refactor annotation checking to expose errors to pylint

Instead of just storing the error messages as part of `check_results()`, we
also store the type of error. This allows us to expose these errors to pylint
in edx-lint, with the help of a new checker. (to be created)

Author: regisb

CHANGELOG.rst

@@ -11,6 +11,11 @@ Change Log
 
 .. There should always be an "Unreleased" section for changes pending release.
 
+[1.1.0] - 2021-01-28
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+* Refactor annotation checking to make it possible to expose errors via pylint
+
 [1.0.2] - 2021-01-22
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 

code_annotations/__init__.py

@@ -2,4 +2,4 @@
 Extensible tools for parsing annotations in codebases.
 """
 
-__version__ = '1.0.2'
+__version__ = '1.1.0'

code_annotations/annotation_errors.py

@@ -0,0 +1,64 @@
+"""
+List possible annotation error types.
+"""
+from collections import namedtuple
+
+AnnotationErrorType = namedtuple(
+    "AnnotationError", ["message", "symbol", "description"]
+)
+
+# The TYPES list should contain all AnnotationErrorType instances. This list can then be parsed by others, for instance
+# to expose errors to pylint.
+TYPES = []
+
+
+def add_error_type(message, symbol, description):
+    """
+    Create an AnnotationErrorType instance and add it to TYPES.
+    """
+    error_type = AnnotationErrorType(
+        message,
+        symbol,
+        description,
+    )
+    TYPES.append(error_type)
+    if len(TYPES) > 10:
+        # if more than 10 items are created here, numerical IDs generated by edx-lint will overlap with other warning
+        # IDs.
+        raise ValueError("TYPES may not contain more than 10 items")
+    return error_type
+
+
+# It is important to preserve the insertion order of these error types in the TYPES list, as edx-lint uses the error
+# type indices to generate numerical pylint IDs. If the insertion order is changed, the pylint IDs will change too,
+# which might cause incompatibilities down the road. Thus, new items should be added at the end.
+InvalidChoice = add_error_type(
+    '"%s" is not a valid choice for "%s". Expected one of %s.',
+    "annotation-invalid-choice",
+    "Emitted when the value of a choice field is not one of the valid choices",
+)
+DuplicateChoiceValue = add_error_type(
+    '"%s" is already present in this annotation.',
+    "annotation-duplicate-choice-value",
+    "Emitted when duplicate values are found in a choice field",
+)
+MissingChoiceValue = add_error_type(
+    'no value found for "%s". Expected one of %s.',
+    "annotation-missing-choice-value",
+    "Emitted when a choice field does not have any value",
+)
+InvalidToken = add_error_type(
+    "'%s' token does not belong to group '%s'. Expected one of: %s",
+    "annotation-invalid-token",
+    "Emitted when a token is found in a group for which it is not valid",
+)
+DuplicateToken = add_error_type(
+    "found duplicate token '%s'",
+    "annotation-duplicate-token",
+    "Emitted when a token is found twice in a group",
+)
+MissingToken = add_error_type(
+    "missing non-optional annotation: '%s'",
+    "annotation-missing-token",
+    "Emitted when a required token is missing from a group",
+)

code_annotations/base.py

@@ -10,6 +10,7 @@
 import yaml
 from stevedore import named
 
+from code_annotations import annotation_errors
 from code_annotations.exceptions import ConfigurationException
 from code_annotations.helpers import VerboseEcho
 
@@ -314,7 +315,11 @@ def __init__(self, config):
         """
         self.config = config
         self.echo = self.config.echo
+        # errors contains formatted error messages
         self.errors = []
+        # annotation_errors contains (annotation, AnnotationErrorType, args) tuples
+        # This attribute may be parsed by 3rd-parties, such as edx-lint.
+        self.annotation_errors = []
 
     def format_file_results(self, all_results, results):
         """
@@ -377,20 +382,18 @@ def _check_results_choices(self, annotation):
                 if choice not in self.config.choices[token]:
                     self._add_annotation_error(
                         annotation,
-                        '"{}" is not a valid choice for "{}". Expected one of {}.'.format(
-                            choice,
-                            token,
-                            self.config.choices[token]
-                        )
+                        annotation_errors.InvalidChoice,
+                        (choice, token, self.config.choices[token])
                     )
                 elif choice in found_valid_choices:
-                    self._add_annotation_error(annotation, f'"{choice}" is already present in this annotation.')
+                    self._add_annotation_error(annotation, annotation_errors.DuplicateChoiceValue, (choice,))
                 else:
                     found_valid_choices.append(choice)
         else:
             self._add_annotation_error(
                 annotation,
-                'no value found for "{}". Expected one of {}.'.format(token, self.config.choices[token])
+                annotation_errors.MissingChoiceValue,
+                (token, self.config.choices[token])
             )
         return None
 
@@ -502,7 +505,8 @@ def check_group(self, annotations):
                 if token not in group_tokens:
                     self._add_annotation_error(
                         annotation,
-                        "'{}' token does not belong to group '{}'. Expected one of: {}".format(
+                        annotation_errors.InvalidToken,
+                        (
                            token,
                            group_name,
                            group_tokens
@@ -513,7 +517,8 @@ def check_group(self, annotations):
             if token in found_tokens:
                 self._add_annotation_error(
                     annotation,
-                    "found duplicate token '{}'".format(token)
+                    annotation_errors.DuplicateToken,
+                    (token,)
                 )
             if group_name:
                 found_tokens.add(token)
@@ -524,69 +529,26 @@ def check_group(self, annotations):
                 if token not in found_tokens:
                     self._add_annotation_error(
                         annotations[0],
-                        "missing non-optional annotation: '{}'".format(token)
-                    )
-
-    def check_annotation(self, annotation, current_group):
-        """
-        Check an annotation and add annotation errors when necessary.
-
-        Args:
-            annotation (dict): in particular, every annotation contains 'annotation_token' and 'annotation_data' keys.
-            current_group (str): None or the name of a group (from self.config.groups) to which preceding annotations
-                belong.
-            found_group_tokens (list): annotation tokens from the same group that were already found. This list is
-                cleared in case of error or when creating a new group.
-
-        Return:
-            current_group (str or None)
-        """
-        self._check_results_choices(annotation)
-        token = annotation['annotation_token']
-
-        if current_group:
-            # Add to existing group
-            if token not in self.config.groups[current_group]:
-                # Check for token correctness
-                self._add_annotation_error(
-                    annotation,
-                    '"{}" is not in the group that starts with "{}". Expecting one of: {}'.format(
-                       token,
-                       current_group,
-                       self.config.groups[current_group]
+                        annotation_errors.MissingToken,
+                        (token,)
                     )
-                )
-                current_group = None
-            else:
-                # Token is correct
-                self.echo.echo_vv('Adding "{}", line {} to group {}'.format(
-                    token,
-                    annotation['line_number'],
-                    current_group
-                ))
-        else:
-            current_group = self._get_group_for_token(token)
-            if current_group:
-                # Start a new group
-                self.echo.echo_vv('Starting new group for "{}" token "{}", line {}'.format(
-                    current_group, token, annotation['line_number'])
-                )
-
-        return current_group
 
-    def _add_annotation_error(self, annotation, message):
+    def _add_annotation_error(self, annotation, error_type, args=None):
         """
         Add an error message to self.errors, formatted nicely.
 
         Args:
             annotation: A single annotation dict found in search()
-            message: Custom error message to be added
-        """
-        if 'extra' in annotation and 'object_id' in annotation['extra']:
-            error = "{}::{}: {}".format(annotation['filename'], annotation['extra']['object_id'], message)
-        else:
-            error = "{}::{}: {}".format(annotation['filename'], annotation['line_number'], message)
-        self.errors.append(error)
+            error_type (annotation_errors.AnnotationErrorType): error type from which the error message will be
+                generated.
+            args (tuple): arguments for error message formatting.
+        """
+        args = args or tuple()
+        error_message = error_type.message % args
+        location = annotation.get("extra", {}).get("object_id", annotation["line_number"])
+        message = "{}::{}: {}".format(annotation['filename'], location, error_message)
+        self.annotation_errors.append((annotation, error_type, args))
+        self._add_error(message)
 
     def _add_error(self, message):
         """

tests/test_search.py

@@ -0,0 +1,45 @@
+"""
+Tests for the StaticSearch/DjangoSearch API.
+"""
+
+from code_annotations import annotation_errors
+from code_annotations.base import AnnotationConfig
+from code_annotations.find_static import StaticSearch
+
+
+def test_annotation_errors():
+    config = AnnotationConfig(
+        "tests/test_configurations/.annotations_test",
+        verbosity=-1,
+        source_path_override="tests/extensions/python_test_files/choice_failures_1.pyt",
+    )
+    search = StaticSearch(config)
+    results = search.search()
+    search.check_results(results)
+
+    # The first error should be an invalid choice error
+    annotation, error_type, args = search.annotation_errors[0]
+    assert {
+        "annotation_data": ["doesnotexist"],
+        "annotation_token": ".. ignored:",
+        "filename": "choice_failures_1.pyt",
+        "found_by": "python",
+        "line_number": 1,
+    } == annotation
+    assert annotation_errors.InvalidChoice == error_type
+    assert (
+        "doesnotexist",
+        ".. ignored:",
+        ["irrelevant", "terrible", "silly-silly"],
+    ) == args
+
+
+def test_annotation_errors_ordering():
+    # You should modify the value below every time a new annotation error type is added.
+    assert 6 == len(annotation_errors.TYPES)
+    # The value below must not be modified, ever. The number of annotation error types should NEVER exceed 10. Read the
+    # module docs for more information.
+    assert len(annotation_errors.TYPES) < 10
+    # This is just to check that the ordering of the annotation error types does not change. You should not change this
+    # test, but eventually add your own below.
+    assert annotation_errors.MissingToken == annotation_errors.TYPES[5]