[primer] Separate false positive sections in primer comment

New useless-suppression messages get a 🎉 section (fixed FP), and new
locally-disabled messages get a 😞 section (introduced FP).

Author: Pierre-Sassoulas

pylint/testutils/_primer/primer_compare_command.py

@@ -3,6 +3,8 @@
 # Copyright (c) https://github.com/pylint-dev/pylint/blob/main/CONTRIBUTORS.txt
 from __future__ import annotations
 
+import re
+from collections.abc import Callable
 from pathlib import PurePosixPath
 
 from pylint.reporters.json_reporter import OldJsonExport
@@ -14,6 +16,66 @@
 from pylint.testutils._primer.primer_command import PrimerCommand
 
 MAX_GITHUB_COMMENT_LENGTH = 65536
+USELESS_SUPPRESSION_RE = re.compile(r"Useless suppression of '(.+)'")
+
+
+class _ClassifiedMessages:
+    """All message categories for a single package, pre-formatted for display."""
+
+    __slots__ = (
+        "astroid_errors",
+        "changed_messages",
+        "fixed_false_positives",
+        "missing_messages",
+        "new_false_positives",
+        "new_messages",
+    )
+
+    def __init__(
+        self,
+        new_messages: list[OldJsonExport],
+        missing_messages: list[OldJsonExport],
+        changed_messages: list[ChangedMessage],
+        source_link: Callable[[OldJsonExport], str],
+    ) -> None:
+        astroid_errors = 0
+        new_fp: list[OldJsonExport] = []
+        fixed_fp: list[OldJsonExport] = []
+        other_new: list[OldJsonExport] = []
+        for message in new_messages:
+            if message["symbol"] == "astroid-error":
+                astroid_errors += 1
+            elif USELESS_SUPPRESSION_RE.match(message["message"]):
+                fixed_fp.append(message)
+            elif message["symbol"] == "locally-disabled":
+                # A new locally-disabled means a maintainer had to add a
+                # suppression comment — we introduced a false positive.
+                new_fp.append(message)
+            else:
+                other_new.append(message)
+
+        def _format(msgs: list[OldJsonExport]) -> str:
+            body = ""
+            for count, msg in enumerate(msgs, 1):
+                body += (
+                    f"{count}) {msg['symbol']}:\n*{msg['message']}*\n"
+                    f"{source_link(msg)}\n"
+                )
+            return body
+
+        self.astroid_errors = astroid_errors
+        self.fixed_false_positives = _format(fixed_fp)
+        self.new_false_positives = _format(new_fp)
+        self.new_messages = _format(other_new)
+        self.missing_messages = _format(missing_messages)
+
+        body = ""
+        for count, (old, new) in enumerate(changed_messages, 1):
+            body += (
+                f"{count}) [{new['symbol']}]({source_link(new)}):\n"
+                f"{message_diff(old, new)}\n"
+            )
+        self.changed_messages = body
 
 
 class CompareCommand(PrimerCommand):
@@ -70,58 +132,41 @@ def _details_section(title: str, body: str) -> str:
             # Two line breaks required after <details> for links to work.
             return f"{title}\n\n<details>\n\n{body}</details>\n\n"
 
+        classified = _ClassifiedMessages(
+            new_messages, missing_messages, changed_messages, _source_link
+        )
         comment = f"\n**Effect on [{package}]({url}):**\n\n"
 
-        # -- Changed messages (same diagnostic, different details) -----------
-        if changed_messages:
-            body = ""
-            for count, (old, new) in enumerate(changed_messages, 1):
-                body += (
-                    f"{count}) [{new['symbol']}]({_source_link(new)}):\n"
-                    f"{message_diff(old, new)}\n"
-                )
+        if classified.changed_messages:
             comment += _details_section(
-                "The following messages have been changed:", body
+                "The following messages have been changed:",
+                classified.changed_messages,
             )
-
-        # -- New messages ----------------------------------------------------
-        astroid_errors = 0
-        new_non_astroid_messages = ""
-        count = 0
-        for message in new_messages:
-            # Existing astroid errors may still show up as "new" because the
-            # timestamp in the message is slightly different.
-            if message["symbol"] == "astroid-error":
-                astroid_errors += 1
-            else:
-                count += 1
-                new_non_astroid_messages += (
-                    f"{count}) {message['symbol']}:\n*{message['message']}*\n"
-                    f"{_source_link(message)}\n"
-                )
-
-        if astroid_errors:
+        if classified.astroid_errors:
             comment += (
-                f'{astroid_errors} "astroid error(s)" were found. '
+                f'{classified.astroid_errors} "astroid error(s)" were found. '
                 "Please open the GitHub Actions log to see what failed or crashed.\n\n"
             )
-        if new_non_astroid_messages:
+        if classified.fixed_false_positives:
             comment += _details_section(
-                "The following messages are now emitted:", new_non_astroid_messages
+                "🎉 The following false positives have been fixed:",
+                classified.fixed_false_positives,
             )
-
-        # -- Missing messages ------------------------------------------------
-        if missing_messages:
-            body = ""
-            for count, message in enumerate(missing_messages, 1):
-                body += (
-                    f"{count}) {message['symbol']}:\n*{message['message']}*\n"
-                    f"{_source_link(message)}\n"
-                )
+        if classified.new_false_positives:
             comment += _details_section(
-                "The following messages are no longer emitted:", body
+                "😞 The following false positives have been introduced:",
+                classified.new_false_positives,
+            )
+        if classified.new_messages:
+            comment += _details_section(
+                "The following messages are now emitted:",
+                classified.new_messages,
+            )
+        if classified.missing_messages:
+            comment += _details_section(
+                "The following messages are no longer emitted:",
+                classified.missing_messages,
             )
-
         return comment
 
     def _truncate_comment(self, comment: str) -> str:

tests/testutils/_primer/cases/batched/expected.txt

@@ -6,6 +6,7 @@
 The following messages are no longer emitted:
 
 <details>
+
 1) locally-disabled:
 *Locally disabling redefined-builtin (W0622)*
 https://github.com/pylint-dev/astroid/blob/1234567890abcdef/astroid/__init__.py#L91

tests/testutils/_primer/cases/new_message/expected.txt

@@ -3,7 +3,7 @@
 
 **Effect on [astroid](https://github.com/pylint-dev/astroid):**
 
-The following messages are now emitted:
+😞 The following false positives have been introduced:
 
 <details>
 

tests/testutils/_primer/cases/useless_suppression/expected.txt

@@ -3,16 +3,22 @@
 
 **Effect on [astropy](https://github.com/astropy/astropy):**
 
-The following messages are now emitted:
+🎉 The following false positives have been fixed:
+
+<details>
+
+1) useless-suppression:
+*Useless suppression of 'looping-through-iterator'*
+https://github.com/astropy/astropy/blob/1fb40bc1f22f176254ef583065aa155f53f3b414/astropy/coordinates/angles/angle_parsetab.py#L14
+</details>
+
+😞 The following false positives have been introduced:
 
 <details>
 
 1) locally-disabled:
 *Locally disabling looping-through-iterator (W4801)*
 https://github.com/astropy/astropy/blob/1fb40bc1f22f176254ef583065aa155f53f3b414/astropy/coordinates/angles/angle_parsetab.py#L14
-2) useless-suppression:
-*Useless suppression of 'looping-through-iterator'*
-https://github.com/astropy/astropy/blob/1fb40bc1f22f176254ef583065aa155f53f3b414/astropy/coordinates/angles/angle_parsetab.py#L14
 </details>
 
 *This comment was generated for commit v2.14.2*