Implement optional fields

This causes backward incompatibility, as we no longer accept annotation groups
of the form:

    """
    .. annotation1: ...
    """
    # .. annotation2: ...

In order to implement this feature, we had to refactor linting.
`check_result()` was extremely convoluted; we both made it clearer and improved
the wording of error messages.

We took the opportunity to improve error messages.

Author: regisb

code_annotations/base.py

@@ -31,6 +31,7 @@ def __init__(self, config_file_path, report_path_override=None, verbosity=1, sou
         """
         self.groups = {}
         self.choices = {}
+        self.optional_groups = []
         self.annotation_tokens = []
         self.annotation_regexes = []
         self.mgr = None
@@ -108,19 +109,36 @@ def _is_choice_group(self, token_or_group):
         Returns:
             True if the type of the annotation is correct for a choice group, otherwise False
         """
-        return isinstance(token_or_group, dict)
+        return isinstance(token_or_group, dict) and "choices" in token_or_group
+
+    def _is_optional_group(self, token_or_group):
+        """
+        Determine if an annotation is an optional group.
+
+        Args:
+            token_or_group: The annotation being checked
+
+        Returns:
+            True if the annotation is optional, otherwise False.
+        """
+        return isinstance(token_or_group, dict) and bool(token_or_group.get("optional"))
 
     def _is_annotation_token(self, token_or_group):
         """
-        Determine if an annotation is a free-form text type.
+        Determine if an annotation has the right format.
 
         Args:
             token_or_group: The annotation being checked
 
         Returns:
             True if the type of the annotation is correct for a text type, otherwise False
         """
-        return token_or_group is None
+        if token_or_group is None:
+            return True
+        if isinstance(token_or_group, dict):
+            # If annotation is a dict, only a few keys are tolerated
+            return set(token_or_group.keys()).issubset({"choices", "optional"})
+        return False
 
     def _add_annotation_token(self, token):
         if token in self.annotation_tokens:
@@ -172,13 +190,15 @@ def _configure_group(self, group_name, group):
             for annotation_token in annotation:
                 annotation_value = annotation[annotation_token]
 
+                # Otherwise it should be a text type, if not then error out
+                if not self._is_annotation_token(annotation_value):
+                    raise ConfigurationException(f'{annotation} is an unknown annotation type.')
                 # The annotation comment is a choice group
                 if self._is_choice_group(annotation_value):
                     self._configure_choices(annotation_token, annotation_value)
-
-                # Otherwise it should be a text type, if not then error out
-                elif not self._is_annotation_token(annotation_value):
-                    raise ConfigurationException(f'{annotation} is an unknown annotation type.')
+                # The annotation comment is not mandatory
+                if self._is_optional_group(annotation_value):
+                    self.optional_groups.append(annotation_token)
 
                 self.groups[group_name].append(annotation_token)
                 self._add_annotation_token(annotation_token)
@@ -370,7 +390,7 @@ def _check_results_choices(self, annotation):
         else:
             self._add_annotation_error(
                 annotation,
-                'No choices found for "{}". Expected one of {}.'.format(token, self.config.choices[token])
+                'no value found for "{}". Expected one of {}.'.format(token, self.config.choices[token])
             )
         return None
 
@@ -419,17 +439,86 @@ def check_results(self, all_results):
 
         # Spin through the search results
         for filename in all_results:
-            current_group = None
-            found_group_tokens = []
-            for annotation in all_results[filename]:
-                current_group = self.check_annotation(annotation, current_group, found_group_tokens)
+            for annotations in self.iter_groups(all_results[filename]):
+                self.check_group(annotations)
+        return not self.errors
 
-            if current_group:
-                self.errors.append('File("{}") finished with an incomplete group {}!'.format(filename, current_group))
+    def iter_groups(self, annotations):
+        """
+        Iterate on groups of annotations. Annotations are considered as a group when they all have the same
+        `line_number`, which should point to the beginning of the annotation group.
 
-        return not self.errors
+        Yield:
+            annotations (annotation list)
+        """
+        current_group = []
+        current_line_number = None
+        for annotation in annotations:
+            line_number = annotation["line_number"]
+            line_number_changed = line_number != current_line_number
+            if line_number_changed:
+                if current_group:
+                    yield current_group
+                current_group.clear()
+            current_group.append(annotation)
+            current_line_number = line_number
+
+        if current_group:
+            yield current_group
 
-    def check_annotation(self, annotation, current_group, found_group_tokens):
+    def check_group(self, annotations):
+        """
+        Perform several linting checks on a group of annotations:
+
+        - Choice fields should have a valid value
+        - Annotation tokens are valid
+        - There is no duplicate
+        - All non-optional tokens are present
+        """
+        found_tokens = set()
+        group_tokens = []
+        group_name = None
+        for annotation in annotations:
+            token = annotation["annotation_token"]
+            if not group_name:
+                group_name = self._get_group_for_token(token)
+                if group_name:
+                    group_tokens = self.config.groups[group_name]
+
+            # Check if choice field
+            self._check_results_choices(annotation)
+
+            # Check token belongs to group
+            if group_name:
+                if token not in group_tokens:
+                    self._add_annotation_error(
+                        annotation,
+                        "'{}' token does not belong to group '{}'. Expected one of: {}".format(
+                           token,
+                           group_name,
+                           group_tokens
+                        )
+                    )
+
+            # Check for duplicates
+            if token in found_tokens:
+                self._add_annotation_error(
+                    annotation,
+                    "found duplicate token '{}'".format(token)
+                )
+            if group_name:
+                found_tokens.add(token)
+
+        # Check for missing tokens
+        for token in group_tokens:
+            if token not in self.config.optional_groups:
+                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.
 
@@ -459,39 +548,21 @@ def check_annotation(self, annotation, current_group, found_group_tokens):
                     )
                 )
                 current_group = None
-                found_group_tokens.clear()
-            elif token in found_group_tokens:
-                # Check for duplicate tokens
-                self._add_annotation_error(
-                    annotation,
-                    '"{}" is already in the group that starts with "{}"'.format(token, current_group)
-                )
-                current_group = None
-                found_group_tokens.clear()
             else:
                 # Token is correct
                 self.echo.echo_vv('Adding "{}", line {} to group {}'.format(
                     token,
                     annotation['line_number'],
                     current_group
                 ))
-                found_group_tokens.append(token)
         else:
             current_group = self._get_group_for_token(token)
             if current_group:
                 # Start a new group
-                found_group_tokens.clear()
-                found_group_tokens.append(token)
                 self.echo.echo_vv('Starting new group for "{}" token "{}", line {}'.format(
                     current_group, token, annotation['line_number'])
                 )
 
-        # If we have all members, this group is done
-        if current_group and len(found_group_tokens) == len(self.config.groups[current_group]):
-            self.echo.echo_vv("Group complete!")
-            current_group = None
-            found_group_tokens.clear()
-
         return current_group
 
     def _add_annotation_error(self, annotation, message):

docs/writing_annotations.rst

@@ -222,3 +222,15 @@ This comment also works even though the ordering is different:
     .. reporting_consumers: Recommendations and email marketing events
     """
 
+If some annotations in a group are optional, you should flag them as such::
+
+  .. code-block:: yaml
+
+      annotations:
+          reporting:
+              - ".. reporting:"
+              - ".. reporting_non_required_field:":
+                  optional: true
+
+Otherwise, linting will trigger an error complaining of a missing annotation when the
+"`.. reporting_non_required_field:`" annotation is not present.

tests/extensions/javascript_test_files/simple_success.js

@@ -1,6 +1,7 @@
-/* .. pii: Group 1 - Annotation 1 */
-// .. pii_types: id, name
-// .. pii_retirement: local_api, consumer_api
+/* .. pii: Group 1 - Annotation 1
+.. pii_types: id, name
+.. pii_retirement: local_api, consumer_api
+*/
 
 'use strict';
 
@@ -13,9 +14,9 @@ var bar = [
     /*
     .. pii: Group 2 - Annotation 1 comment is a
     multi line comment
+    .. pii_types: id, name
+    .. pii_retirement: local_api, consumer_api
     */
-    // .. pii_types: id, name
-    // .. pii_retirement: local_api, consumer_api
     /pii\/js/,
     // .. pii: Group 3 - Annotation 1
     // .. pii_types: id, name

tests/extensions/python_test_files/simple_success.pyt

@@ -3,9 +3,8 @@ Docstring
 
 .. pii: Annotation 1
 .. pii_types: id, name
+.. pii_retirement: local_api, consumer_api
 """
-# Should be able to finish the group outside of the same comment if necessary
-# .. pii_retirement: local_api, consumer_api
 
 
 def do():

tests/extensions/test_extension_javascript.py

@@ -10,15 +10,15 @@
     ('simple_success.js', EXIT_CODE_SUCCESS, 'Search found 26 annotations in'),
     ('group_ordering_1.js', EXIT_CODE_SUCCESS, 'Search found 3 annotations in'),
     ('group_ordering_2.js', EXIT_CODE_SUCCESS, 'Search found 9 annotations in'),
-    ('group_failures_1.js', EXIT_CODE_FAILURE, 'File("group_failures_1.js") finished with an incomplete group'),
-    ('group_failures_2.js', EXIT_CODE_FAILURE, 'File("group_failures_2.js") finished with an incomplete group'),
-    ('group_failures_4.js', EXIT_CODE_FAILURE, '".. no_pii:" is not in the group that starts with'),
-    ('group_failures_5.js', EXIT_CODE_FAILURE, '".. pii_types:" is already in the group that starts with'),
+    ('group_failures_1.js', EXIT_CODE_FAILURE, "missing non-optional annotation: '.. pii_retirement:'"),
+    ('group_failures_2.js', EXIT_CODE_FAILURE, "missing non-optional annotation: '.. pii:"),
+    ('group_failures_4.js', EXIT_CODE_FAILURE, "'.. no_pii:' token does not belong to group 'pii_group'"),
+    ('group_failures_5.js', EXIT_CODE_FAILURE, "found duplicate token '.. pii_types:'"),
     ('choice_failures_1.js', EXIT_CODE_FAILURE, '"doesnotexist" is not a valid choice for ".. ignored:"'),
     ('choice_failures_2.js', EXIT_CODE_FAILURE, '"doesnotexist" is not a valid choice for ".. ignored:"'),
     ('choice_failures_3.js', EXIT_CODE_FAILURE, '"terrible|silly-silly" is not a valid choice for ".. ignored:"'),
     ('choice_failures_4.js', EXIT_CODE_FAILURE, '"terrible" is already present in this annotation'),
-    ('choice_failures_5.js', EXIT_CODE_FAILURE, 'No choices found for ".. ignored:"'),
+    ('choice_failures_5.js', EXIT_CODE_FAILURE, 'no value found for ".. ignored:"'),
 ])
 def test_grouping_and_choice_failures(test_file, expected_exit_code, expected_message):
     result = call_script((

tests/extensions/test_extension_python.py

@@ -13,13 +13,13 @@
     ('simple_success.pyt', EXIT_CODE_SUCCESS, 'Search found 20 annotations in'),
     ('group_ordering_1.pyt', EXIT_CODE_SUCCESS, 'Search found 3 annotations in'),
     ('group_ordering_2.pyt', EXIT_CODE_SUCCESS, 'Search found 9 annotations in'),
-    ('group_failures_1.pyt', EXIT_CODE_FAILURE, 'File("group_failures_1.pyt") finished with an incomplete group'),
-    ('group_failures_2.pyt', EXIT_CODE_FAILURE, 'File("group_failures_2.pyt") finished with an incomplete group'),
+    ('group_failures_1.pyt', EXIT_CODE_FAILURE, "missing non-optional annotation: '.. pii_retirement:'"),
+    ('group_failures_2.pyt', EXIT_CODE_FAILURE, "missing non-optional annotation: '.. pii:'"),
     ('choice_failures_1.pyt', EXIT_CODE_FAILURE, '"doesnotexist" is not a valid choice for ".. ignored:"'),
     ('choice_failures_2.pyt', EXIT_CODE_FAILURE, '"doesnotexist" is not a valid choice for ".. ignored:"'),
     ('choice_failures_3.pyt', EXIT_CODE_FAILURE, '"terrible|silly-silly" is not a valid choice for ".. ignored:"'),
     ('choice_failures_4.pyt', EXIT_CODE_FAILURE, '"terrible" is already present in this annotation'),
-    ('choice_failures_5.pyt', EXIT_CODE_FAILURE, 'No choices found for ".. ignored:"'),
+    ('choice_failures_5.pyt', EXIT_CODE_FAILURE, 'no value found for ".. ignored:"'),
 ])
 def test_grouping_and_choice_failures(test_file, expected_exit_code, expected_message):
     result = call_script((

tests/test_configurations/.annotations_test

@@ -12,6 +12,8 @@ annotations:
             choices: [id, name, other]
         - ".. pii_retirement:":
             choices: [retained, local_api, consumer_api, third_party]
+        - ".. pii_optional:":
+            optional: true
 extensions:
     python:
         - pyt

tests/test_find_django.py

@@ -292,7 +292,7 @@ def test_find_django_ordering_error(**kwargs):
     )
 
     assert result.exit_code == EXIT_CODE_FAILURE
-    assert '".. no_pii:" is not in the group ' in result.output
+    assert "missing non-optional annotation: '.. pii:'" in result.output
 
 
 @patch.multiple(