refactor issues and reporting to handle lint and build issues

bot/code_review_bot/__init__.py

@@ -14,7 +14,7 @@
 
 # Workaround https://github.com/taskcluster/taskcluster/issues/9172
 import taskcluster.download
-from libmozdata.phabricator import LintResult, UnitResult, UnitResultState
+from libmozdata.phabricator import LintResult
 from taskcluster.helper import TaskclusterConfig
 
 from code_review_bot.config import settings
@@ -66,11 +66,134 @@ class Level(enum.Enum):
     Warning = "warning"
 
 
-class Issue(abc.ABC):
+class IssueType(enum.Enum):
+    Lint = 1
+
+
+class BaseIssue(abc.ABC):
+    type_: IssueType
+
+    def __init__(
+        self, analyzer, revision, level: Level = Level.Warning, message: str = ""
+    ):
+        # Check while avoiding circular dependencies
+        from code_review_bot.revisions import Revision
+
+        assert isinstance(revision, Revision)
+        assert isinstance(analyzer, AnalysisTask)
+
+        self.analyzer = analyzer
+        self.revision = revision
+        self.level = level
+        self.message = message
+        # Mark the issue as known by default, so only errors are reported
+        # The before/after feature may tag some issues as new, so they are reported
+        self.new_issue = False
+        # Reserved payload for backend
+        self.on_backend = None
+
+    @property
+    def display_name(self):
+        """
+        Issue's base name (by default analyzer's name)
+        But can be overridden by subclasses
+        """
+        return self.analyzer.display_name
+
+    def build_extra_identifiers(self):
+        """
+        Used to add information when building an issue unique hash
+        """
+        return {}
+
+    @property
+    def allow_before_and_after_publish(self):
+        """
+        Allow the possibility for an issue to avoid being published based on before/after.
+        This allow publishing issues based on other criteria, like in_patch.
+        """
+        if taskcluster.secrets.get(
+            f"{self.analyzer.name.upper()}_DISABLE_PUBLICATION_BEFORE_AFTER", False
+        ):
+            return False
+
+        return self.revision.before_after_feature
+
+    @cached_property
+    def hash(self):
+        raise NotImplementedError
+
+    @abc.abstractmethod
+    def is_publishable(self):
+        raise NotImplementedError
+
+    @abc.abstractmethod
+    def validates(self):
+        """
+        Is this issue publishable on reporters using IN_PATCH publication ?
+        Should check specific rules and return a boolean
+        """
+        raise NotImplementedError
+
+    def as_dict(self):
+        """
+        Build the serializable dict representation of the issue
+        Used by debugging tools
+        """
+        issue_hash = None
+        try:
+            issue_hash = self.hash
+        except Exception as e:
+            logger.warn("Failed to build issue hash", error=str(e), issue=str(self))
+
+        return {
+            "analyzer": self.analyzer.name,
+            "level": self.level.value,
+            "message": self.message,
+            "validates": self.validates(),
+            "publishable": self.is_publishable(),
+            "hash": issue_hash,
+        }
+
+    @abc.abstractmethod
+    def as_text(self):
+        """
+        Build the text content for reporters
+        """
+        raise NotImplementedError
+
+    @abc.abstractmethod
+    def as_markdown(self):
+        """
+        Build the Markdown content for debug email
+        """
+        raise NotImplementedError
+
+    def as_error(self):
+        """
+        Build the Markdown content for for build error issues
+        """
+        raise NotImplementedError
+
+    @abc.abstractmethod
+    def is_build_error(self) -> bool:
+        """
+        Is this issue a build error?
+        Default is False
+        """
+        raise NotImplementedError
+
+    @abc.abstractmethod
+    def as_phabricator_issue(self):
+        raise NotImplementedError
+
+
+class Issue(BaseIssue):
     """
     Common reported issue interface
     """
 
+    type_: IssueType = IssueType.Lint
     revision = None
 
     def __init__(
@@ -82,21 +205,15 @@ def __init__(
         nb_lines: int,
         check: str,
         column: int = None,
-        message: str = None,
+        message: str = "",
         level: Level = Level.Warning,
         fix: str = None,
         language: str = None,
     ):
-        # Check while avoiding circular dependencies
-        from code_review_bot.revisions import Revision
-
-        assert isinstance(revision, Revision)
-        assert isinstance(analyzer, AnalysisTask)
+        super().__init__(analyzer, revision, level, message)
 
         # Base required fields for all issues
         assert not os.path.isabs(path), f"Issue path can not be absolute {path}"
-        self.revision = revision
-        self.analyzer = analyzer
         self.check = check
         self.path = path
         self.line = positive_int("line", line)
@@ -109,52 +226,24 @@ def __init__(
 
         # Optional common fields
         self.column = column
-        self.message = message
-        self.level = level
-
-        # Reserved payload for backend
-        self.on_backend = None
 
         # Store information when a fix is available
         self.fix = fix
         self.language = language
         if self.fix is not None:
             assert self.language is not None, "Missing fix language"
 
-        # Mark the issue as known by default, so only errors are reported
-        # The before/after feature may tag some issues as new, so they are reported
-        self.new_issue = False
-
     def __str__(self):
         line = f"line {self.line}" if self.line is not None else "full file"
         return f"{self.analyzer.name} issue {self.check}@{self.level.value} {self.path} {line}"
 
     @property
-    def display_name(self):
-        """
-        Issue's base name (by default analyzer's name)
-        But can be overridden by subclasses
-        """
-        return self.analyzer.display_name
-
-    def build_extra_identifiers(self):
-        """
-        Used to add information when building an issue unique hash
-        """
-        return {}
+    def in_patch(self):
+        return self.revision.contains(self)
 
     @property
-    def allow_before_and_after_publish(self):
-        """
-        Allow the possibility for an issue to avoid being published based on before/after.
-        This allow publishing issues based on other criteria, like in_patch.
-        """
-        if taskcluster.secrets.get(
-            f"{self.analyzer.name.upper()}_DISABLE_PUBLICATION_BEFORE_AFTER", False
-        ):
-            return False
-
-        return self.revision.before_after_feature
+    def in_touched_files(self):
+        return self.revision.in_touched_files(self)
 
     def is_publishable(self):
         """
@@ -181,14 +270,6 @@ def is_publishable(self):
         # Fallback to in_patch detection
         return self.in_patch
 
-    @property
-    def in_patch(self):
-        return self.revision.contains(self)
-
-    @property
-    def in_touched_files(self):
-        return self.revision.in_touched_files(self)
-
     @cached_property
     def hash(self):
         """
@@ -282,62 +363,23 @@ def file_exists(self):
                     raise e
                 return False
 
-    @abc.abstractmethod
-    def validates(self):
-        """
-        Is this issue publishable on reporters using IN_PATCH publication ?
-        Should check specific rules and return a boolean
-        """
-        raise NotImplementedError
-
-    @abc.abstractmethod
-    def as_text(self):
-        """
-        Build the text content for reporters
-        """
-        raise NotImplementedError
-
-    @abc.abstractmethod
-    def as_markdown(self):
-        """
-        Build the Markdown content for debug email
-        """
-        raise NotImplementedError
-
-    def as_error(self):
-        """
-        Build the Markdown content for for build error issues
-        """
-        raise NotImplementedError
-
     def as_dict(self):
-        """
-        Build the serializable dict representation of the issue
-        Used by debugging tools
-        """
-        issue_hash = None
-        try:
-            issue_hash = self.hash
-        except Exception as e:
-            logger.warn("Failed to build issue hash", error=str(e), issue=str(self))
+        dict_repr = super().as_dict()
+        dict_repr.update(
+            {
+                "path": self.path,
+                "line": self.line,
+                "nb_lines": self.nb_lines,
+                "column": self.column,
+                "check": self.check,
+                "in_patch": self.in_patch,
+                "fix": self.fix,
+            }
+        )
 
-        return {
-            "analyzer": self.analyzer.name,
-            "path": self.path,
-            "line": self.line,
-            "nb_lines": self.nb_lines,
-            "column": self.column,
-            "check": self.check,
-            "level": self.level.value,
-            "message": self.message,
-            "in_patch": self.in_patch,
-            "validates": self.validates(),
-            "publishable": self.is_publishable(),
-            "hash": issue_hash,
-            "fix": self.fix,
-        }
+        return dict_repr
 
-    def as_phabricator_lint(self):
+    def as_phabricator_issue(self):
         """
         Build the Phabricator LintResult instance
         """
@@ -367,27 +409,7 @@ def as_phabricator_lint(self):
             char=self.column,
         )
 
-    def as_phabricator_unitresult(self):
-        """
-        Build a Phabricator UnitResult for build errors
-        """
-        assert (
-            self.is_build_error()
-        ), "Only build errors may be published as unit results"
-
-        return UnitResult(
-            namespace="code-review",
-            name="general",
-            result=UnitResultState.Fail,
-            details=f"Code review bot found a **build error**: \n{self.message}",
-            format="remarkup",
-        )
-
     def is_build_error(self):
-        """
-        Is this issue a build error?
-        Default is False
-        """
         return False