From 64b4008eb62986b70cd9ad2f12af6b7fd72c4ee5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Behmo?= Date: Thu, 28 Jan 2021 11:47:27 +0100 Subject: [PATCH] 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) --- CHANGELOG.rst | 5 ++ code_annotations/__init__.py | 2 +- code_annotations/annotation_errors.py | 64 +++++++++++++++++++ code_annotations/base.py | 92 ++++++++------------------- tests/test_search.py | 45 +++++++++++++ 5 files changed, 142 insertions(+), 66 deletions(-) create mode 100644 code_annotations/annotation_errors.py create mode 100644 tests/test_search.py diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 1a93bc4..1057702 100644 --- a/CHANGELOG.rst +++ b/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 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/code_annotations/__init__.py b/code_annotations/__init__.py index 1fec0ed..ea69c73 100644 --- a/code_annotations/__init__.py +++ b/code_annotations/__init__.py @@ -2,4 +2,4 @@ Extensible tools for parsing annotations in codebases. """ -__version__ = '1.0.2' +__version__ = '1.1.0' diff --git a/code_annotations/annotation_errors.py b/code_annotations/annotation_errors.py new file mode 100644 index 0000000..16fae58 --- /dev/null +++ b/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", +) diff --git a/code_annotations/base.py b/code_annotations/base.py index 8b97277..ee7ac91 100644 --- a/code_annotations/base.py +++ b/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): """ diff --git a/tests/test_search.py b/tests/test_search.py new file mode 100644 index 0000000..45a48c6 --- /dev/null +++ b/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]