Use permanent links in the primer comment (#7332)

pylint-dev · Aug 22, 2022 · d3b88c9 · d3b88c9
1 parent 8e57fe1
commit d3b88c9
Show file tree

Hide file tree

Showing 10 changed files with 194 additions and 157 deletions.
diff --git a/pylint/testutils/_primer/primer_command.py b/pylint/testutils/_primer/primer_command.py
@@ -6,13 +6,25 @@
 
 import abc
 import argparse
+import sys
 from pathlib import Path
-from typing import Dict, List
+from typing import Dict
 
-from pylint.message import Message
+from pylint.reporters.json_reporter import OldJsonExport
 from pylint.testutils._primer import PackageToLint
 
-PackageMessages = Dict[str, List[Message]]
+if sys.version_info >= (3, 8):
+    from typing import TypedDict
+else:
+    from typing_extensions import TypedDict
+
+
+class PackageData(TypedDict):
+    commit: str
+    messages: list[OldJsonExport]
+
+
+PackageMessages = Dict[str, PackageData]
 
 
 class PrimerCommand:

diff --git a/pylint/testutils/_primer/primer_compare_command.py b/pylint/testutils/_primer/primer_compare_command.py
@@ -4,37 +4,45 @@
 from __future__ import annotations
 
 import json
-from pathlib import Path
+from pathlib import Path, PurePosixPath
 
-from pylint.testutils._primer.primer_command import PackageMessages, PrimerCommand
+from pylint.reporters.json_reporter import OldJsonExport
+from pylint.testutils._primer.primer_command import (
+    PackageData,
+    PackageMessages,
+    PrimerCommand,
+)
 
 MAX_GITHUB_COMMENT_LENGTH = 65536
 
 
 class CompareCommand(PrimerCommand):
     def run(self) -> None:
-        main_messages = self._load_json(self.config.base_file)
-        pr_messages = self._load_json(self.config.new_file)
-        missing_messages, new_messages = self._cross_reference(
-            main_messages, pr_messages
+        main_data = self._load_json(self.config.base_file)
+        pr_data = self._load_json(self.config.new_file)
+        missing_messages_data, new_messages_data = self._cross_reference(
+            main_data, pr_data
         )
-        comment = self._create_comment(missing_messages, new_messages)
+        comment = self._create_comment(missing_messages_data, new_messages_data)
         with open(self.primer_directory / "comment.txt", "w", encoding="utf-8") as f:
             f.write(comment)
 
     @staticmethod
     def _cross_reference(
-        main_dict: PackageMessages, pr_messages: PackageMessages
+        main_data: PackageMessages, pr_data: PackageMessages
     ) -> tuple[PackageMessages, PackageMessages]:
-        missing_messages: PackageMessages = {}
-        for package, messages in main_dict.items():
-            missing_messages[package] = []
-            for message in messages:
+        missing_messages_data: PackageMessages = {}
+        for package, data in main_data.items():
+            package_missing_messages: list[OldJsonExport] = []
+            for message in data["messages"]:
                 try:
-                    pr_messages[package].remove(message)
+                    pr_data[package]["messages"].remove(message)
                 except ValueError:
-                    missing_messages[package].append(message)
-        return missing_messages, pr_messages
+                    package_missing_messages.append(message)
+            missing_messages_data[package] = PackageData(
+                commit=pr_data[package]["commit"], messages=package_missing_messages
+            )
+        return missing_messages_data, pr_data
 
     @staticmethod
     def _load_json(file_path: Path | str) -> PackageMessages:
@@ -50,7 +58,7 @@ def _create_comment(
             if len(comment) >= MAX_GITHUB_COMMENT_LENGTH:
                 break
             new_messages = all_new_messages[package]
-            if not missing_messages and not new_messages:
+            if not missing_messages["messages"] and not new_messages["messages"]:
                 continue
             comment += self._create_comment_for_package(
                 package, new_messages, missing_messages
@@ -67,18 +75,20 @@ def _create_comment(
         return self._truncate_comment(comment)
 
     def _create_comment_for_package(
-        self, package: str, new_messages, missing_messages
+        self, package: str, new_messages: PackageData, missing_messages: PackageData
     ) -> str:
         comment = f"\n\n**Effect on [{package}]({self.packages[package].url}):**\n"
         # Create comment for new messages
         count = 1
         astroid_errors = 0
         new_non_astroid_messages = ""
-        if new_messages:
+        if new_messages["messages"]:
             print("Now emitted:")
-        for message in new_messages:
-            filepath = str(message["path"]).replace(
-                str(self.packages[package].clone_directory), ""
+        for message in new_messages["messages"]:
+            filepath = str(
+                PurePosixPath(message["path"]).relative_to(
+                    self.packages[package].clone_directory
+                )
             )
             # Existing astroid errors may still show up as "new" because the timestamp
             # in the message is slightly different.
@@ -87,7 +97,7 @@ def _create_comment_for_package(
             else:
                 new_non_astroid_messages += (
                     f"{count}) {message['symbol']}:\n*{message['message']}*\n"
-                    f"{self.packages[package].url}/blob/{self.packages[package].branch}{filepath}#L{message['line']}\n"
+                    f"{self.packages[package].url}/blob/{new_messages['commit']}/{filepath}#L{message['line']}\n"
                 )
                 print(message)
                 count += 1
@@ -106,18 +116,20 @@ def _create_comment_for_package(
 
         # Create comment for missing messages
         count = 1
-        if missing_messages:
+        if missing_messages["messages"]:
             comment += "The following messages are no longer emitted:\n\n<details>\n\n"
             print("No longer emitted:")
-        for message in missing_messages:
+        for message in missing_messages["messages"]:
             comment += f"{count}) {message['symbol']}:\n*{message['message']}*\n"
-            filepath = str(message["path"]).replace(
-                str(self.packages[package].clone_directory), ""
+            filepath = str(
+                PurePosixPath(message["path"]).relative_to(
+                    self.packages[package].clone_directory
+                )
             )
             assert not self.packages[package].url.endswith(
                 ".git"
             ), "You don't need the .git at the end of the github url."
-            comment += f"{self.packages[package].url}/blob/{self.packages[package].branch}{filepath}#L{message['line']}\n"
+            comment += f"{self.packages[package].url}/blob/{new_messages['commit']}/{filepath}#L{message['line']}\n"
             count += 1
             print(message)
         if missing_messages:

diff --git a/pylint/testutils/_primer/primer_run_command.py b/pylint/testutils/_primer/primer_run_command.py
@@ -9,25 +9,32 @@
 import warnings
 from io import StringIO
 
+from git.repo import Repo
+
 from pylint.lint import Run
 from pylint.message import Message
 from pylint.reporters import JSONReporter
 from pylint.reporters.json_reporter import OldJsonExport
 from pylint.testutils._primer.package_to_lint import PackageToLint
-from pylint.testutils._primer.primer_command import PrimerCommand
+from pylint.testutils._primer.primer_command import (
+    PackageData,
+    PackageMessages,
+    PrimerCommand,
+)
 
 GITHUB_CRASH_TEMPLATE_LOCATION = "/home/runner/.cache"
 CRASH_TEMPLATE_INTRO = "There is a pre-filled template"
 
 
 class RunCommand(PrimerCommand):
     def run(self) -> None:
-        packages: dict[str, list[OldJsonExport]] = {}
+        packages: PackageMessages = {}
         fatal_msgs: list[Message] = []
         for package, data in self.packages.items():
             messages, p_fatal_msgs = self._lint_package(package, data)
             fatal_msgs += p_fatal_msgs
-            packages[package] = messages
+            local_commit = Repo(data.clone_directory).head.object.hexsha
+            packages[package] = PackageData(commit=local_commit, messages=messages)
         path = (
             self.primer_directory
             / f"output_{'.'.join(str(i) for i in sys.version_info[:3])}_{self.config.type}.txt"

diff --git a/tests/testutils/_primer/fixtures/message_changed/expected.txt b/tests/testutils/_primer/fixtures/message_changed/expected.txt
@@ -9,7 +9,7 @@ The following messages are now emitted:
 
 1) locally-disabled:
 *Locally disabling redefined-builtin [we added some text in the message] (W0622)*
-https://github.com/PyCQA/astroid/blob/main/astroid/__init__.py#L91
+https://github.com/PyCQA/astroid/blob/123456789abcdef/astroid/__init__.py#L91
 
 </details>
 
@@ -19,7 +19,7 @@ The following messages are no longer emitted:
 
 1) locally-disabled:
 *Locally disabling redefined-builtin (W0622)*
-https://github.com/PyCQA/astroid/blob/main/astroid/__init__.py#L91
+https://github.com/PyCQA/astroid/blob/123456789abcdef/astroid/__init__.py#L91
 
 </details>
 

diff --git a/tests/testutils/_primer/fixtures/message_changed/expected_truncated.txt b/tests/testutils/_primer/fixtures/message_changed/expected_truncated.txt
@@ -9,12 +9,12 @@ The following messages are now emitted:
 
 1) locally-disabled:
 *Locally disabling redefined-builtin [we added some text in the message] (W0622)*
-https://github.com/PyCQA/astroid/blob/main/astroid/__init__.py#L91
+https://github.com/PyCQA/astroid/blob/123456789abcdef/astroid/__init__.py#L91
 
 </details>
 
-The fol...
+The following message...
 
-*This comment was truncated because GitHub allows only 500 characters in a comment.*
+*This comment was truncated because GitHub allows only 525 characters in a comment.*
 
 *This comment was generated for commit v2.14.2*
diff --git a/tests/testutils/_primer/fixtures/message_changed/main.json b/tests/testutils/_primer/fixtures/message_changed/main.json
@@ -1,30 +1,33 @@
 {
-  "astroid": [
-    {
-      "type": "info",
-      "module": "astroid",
-      "obj": "",
-      "line": 91,
-      "column": 0,
-      "endLine": null,
-      "endColumn": null,
-      "path": "tests/.pylint_primer_tests/PyCQA/astroid/astroid/__init__.py",
-      "symbol": "locally-disabled",
-      "message": "Locally disabling redefined-builtin (W0622)",
-      "message-id": "I0011"
-    },
-    {
-      "type": "warning",
-      "module": "astroid",
-      "obj": "",
-      "line": 91,
-      "column": 0,
-      "endLine": null,
-      "endColumn": null,
-      "path": "tests/.pylint_primer_tests/PyCQA/astroid/astroid/__init__.py",
-      "symbol": "unknown-option-value",
-      "message": "Unknown option value for 'disable', expected a valid pylint message and got 'Ellipsis'",
-      "message-id": "W0012"
-    }
-  ]
+  "astroid": {
+    "commit": "123456789abcdef",
+    "messages": [
+      {
+        "type": "info",
+        "module": "astroid",
+        "obj": "",
+        "line": 91,
+        "column": 0,
+        "endLine": null,
+        "endColumn": null,
+        "path": "tests/.pylint_primer_tests/PyCQA/astroid/astroid/__init__.py",
+        "symbol": "locally-disabled",
+        "message": "Locally disabling redefined-builtin (W0622)",
+        "message-id": "I0011"
+      },
+      {
+        "type": "warning",
+        "module": "astroid",
+        "obj": "",
+        "line": 91,
+        "column": 0,
+        "endLine": null,
+        "endColumn": null,
+        "path": "tests/.pylint_primer_tests/PyCQA/astroid/astroid/__init__.py",
+        "symbol": "unknown-option-value",
+        "message": "Unknown option value for 'disable', expected a valid pylint message and got 'Ellipsis'",
+        "message-id": "W0012"
+      }
+    ]
+  }
 }
diff --git a/tests/testutils/_primer/fixtures/message_changed/pr.json b/tests/testutils/_primer/fixtures/message_changed/pr.json
@@ -1,30 +1,33 @@
 {
-  "astroid": [
-    {
-      "type": "info",
-      "module": "astroid",
-      "obj": "",
-      "line": 91,
-      "column": 0,
-      "endLine": null,
-      "endColumn": null,
-      "path": "tests/.pylint_primer_tests/PyCQA/astroid/astroid/__init__.py",
-      "symbol": "locally-disabled",
-      "message": "Locally disabling redefined-builtin [we added some text in the message] (W0622)",
-      "message-id": "I0011"
-    },
-    {
-      "type": "warning",
-      "module": "astroid",
-      "obj": "",
-      "line": 91,
-      "column": 0,
-      "endLine": null,
-      "endColumn": null,
-      "path": "tests/.pylint_primer_tests/PyCQA/astroid/astroid/__init__.py",
-      "symbol": "unknown-option-value",
-      "message": "Unknown option value for 'disable', expected a valid pylint message and got 'Ellipsis'",
-      "message-id": "W0012"
-    }
-  ]
+  "astroid": {
+    "commit": "123456789abcdef",
+    "messages": [
+      {
+        "type": "info",
+        "module": "astroid",
+        "obj": "",
+        "line": 91,
+        "column": 0,
+        "endLine": null,
+        "endColumn": null,
+        "path": "tests/.pylint_primer_tests/PyCQA/astroid/astroid/__init__.py",
+        "symbol": "locally-disabled",
+        "message": "Locally disabling redefined-builtin [we added some text in the message] (W0622)",
+        "message-id": "I0011"
+      },
+      {
+        "type": "warning",
+        "module": "astroid",
+        "obj": "",
+        "line": 91,
+        "column": 0,
+        "endLine": null,
+        "endColumn": null,
+        "path": "tests/.pylint_primer_tests/PyCQA/astroid/astroid/__init__.py",
+        "symbol": "unknown-option-value",
+        "message": "Unknown option value for 'disable', expected a valid pylint message and got 'Ellipsis'",
+        "message-id": "W0012"
+      }
+    ]
+  }
 }