From a5ba514dcf070f0875fc422a64b1811a16ddf943 Mon Sep 17 00:00:00 2001 From: Pierre Sassoulas Date: Thu, 2 Apr 2026 22:48:10 +0200 Subject: [PATCH 1/8] [primer] Detect changed messages via position-based pairing Instead of reporting a moved or reworded diagnostic as a separate removal + addition, pair messages that share the same (symbol, path, obj) key and report them as "changed" with a human-readable diff. Comparator.__iter__ now yields 4-tuples (package, missing, new, changed) and exposes a commits dict. Short field changes (type, confidence) use an inline format; long message strings get a diff block with caret hints highlighting the changed spans. --- pylint/testutils/_primer/comparator.py | 158 ++++++++++++++++-- .../_primer/primer_compare_command.py | 83 +++++---- .../batched_cases/expected_comparator.json | 2 +- .../cases/confidence_changed/expected.txt | 16 +- .../expected_comparator.json | 2 +- .../_primer/cases/line_moved/expected.txt | 16 +- .../cases/line_moved/expected_comparator.json | 2 +- .../cases/message_changed/expected.txt | 20 +-- .../message_changed/expected_comparator.json | 2 +- .../message_changed/expected_truncated.txt | 8 +- .../expected_truncated_in_details.txt | 5 +- .../cases/message_changed_line/expected.txt | 20 +-- .../expected_comparator.json | 2 +- .../_primer/cases/mixed_changes/expected.txt | 38 ++--- .../mixed_changes/expected_comparator.json | 2 +- .../_primer/cases/mixed_changes/pr.json | 4 +- .../_primer/cases/multi_package/expected.txt | 2 +- .../multi_package/expected_comparator.json | 4 +- .../new_message/expected_comparator.json | 2 +- .../removed_message/expected_comparator.json | 2 +- .../_primer/cases/type_changed/expected.txt | 16 +- .../type_changed/expected_comparator.json | 2 +- .../expected_comparator.json | 2 +- tests/testutils/_primer/test_comparator.py | 6 +- 24 files changed, 257 insertions(+), 159 deletions(-) diff --git a/pylint/testutils/_primer/comparator.py b/pylint/testutils/_primer/comparator.py index 655a050f04..0b09a918ff 100644 --- a/pylint/testutils/_primer/comparator.py +++ b/pylint/testutils/_primer/comparator.py @@ -5,15 +5,24 @@ from __future__ import annotations import json +from collections import defaultdict from collections.abc import Iterator +from difflib import SequenceMatcher from pathlib import Path from typing import NamedTuple from pylint.reporters.json_reporter import JSONMessage -from pylint.testutils._primer.primer_command import ( - PackageData, - PackageMessages, -) +from pylint.testutils._primer.primer_command import PackageData, PackageMessages + + +class ChangedMessage(NamedTuple): + """A message that was present on both runs but with altered details.""" + + old: JSONMessage + new: JSONMessage + + +_LOCATION_KEYS = {"line", "column", "endLine", "endColumn"} class PackageDiff(NamedTuple): @@ -22,10 +31,120 @@ class PackageDiff(NamedTuple): package: str missing: PackageData # emitted on main but not on the PR new: PackageData # emitted on the PR but not on main + changed: list[ChangedMessage] # same diagnostic, altered line / text + + +def _match_residuals( + old_residuals: list[JSONMessage], new_residuals: list[JSONMessage] +) -> tuple[list[ChangedMessage], list[JSONMessage], list[JSONMessage]]: + """Pair residual messages by ``(symbol, path, obj)``. + + Two messages sharing that key are the "same diagnostic" — if they differ + only in line numbers or message text, they should be reported as *changed* + rather than as a separate removal + addition. Leftovers on each side are + genuinely missing or genuinely new. + + Returns ``(paired, unmatched_old, unmatched_new)``. + """ + old_by_key: dict[tuple[str, str, str], list[JSONMessage]] = defaultdict(list) + new_by_key: dict[tuple[str, str, str], list[JSONMessage]] = defaultdict(list) + for m in old_residuals: + old_by_key[m["symbol"], m["path"], m["obj"]].append(m) + for m in new_residuals: + new_by_key[m["symbol"], m["path"], m["obj"]].append(m) + + paired: list[ChangedMessage] = [] + paired_old_ids: set[int] = set() + paired_new_ids: set[int] = set() + for key in old_by_key: + if key not in new_by_key: + continue + for old, new in zip(old_by_key[key], new_by_key[key]): + paired.append(ChangedMessage(old=old, new=new)) + paired_old_ids.add(id(old)) + paired_new_ids.add(id(new)) + + final_missing = [m for m in old_residuals if id(m) not in paired_old_ids] + final_new = [m for m in new_residuals if id(m) not in paired_new_ids] + return paired, final_missing, final_new + + +def format_span(msg: JSONMessage) -> str: + """Format a message's location as ``line:col to endLine:endCol``.""" + start = f"{msg['line']}:{msg['column']}" + end_line = msg.get("endLine") + end_col = msg.get("endColumn") + if end_line is not None and end_col is not None: + return f"from `{start}` to `{end_line}:{end_col}`" + return f"at `{start}`" + + +def message_diff(old: JSONMessage, new: JSONMessage) -> str: + """Return a compact summary of changed fields between two messages. + + Location changes are merged into a single human-readable span. + String fields (like ``message``) get a ``diff`` code block so GitHub + renders them with red/green highlighting. + """ + changed_keys: set[str] = set() + for key in old: + if old[key] != new[key]: # type: ignore[literal-required] + changed_keys.add(key) + + parts: list[str] = [] + # Location: combine line/column/endLine/endColumn into one sentence. + if changed_keys & _LOCATION_KEYS: + parts.append(f"Was raised {format_span(old)}, now {format_span(new)}.") + + # Other fields (typically ``message`` or ``type``). + for key in sorted(changed_keys - _LOCATION_KEYS): + old_val = old[key] # type: ignore[literal-required] + new_val = new[key] # type: ignore[literal-required] + if ( + isinstance(old_val, str) + and isinstance(new_val, str) + and (len(old_val) > 40 or len(new_val) > 40) + ): + caret_line = _caret_hint(old_val, new_val) + diff_block = f"```diff\n- {old_val}\n+ {new_val}\n" + if caret_line: + diff_block += f" {caret_line}\n" + diff_block += "```" + parts.append(diff_block) + else: + parts.append(f"{key}: `{old_val}` → `{new_val}`") + return "\n\n".join(parts) + + +def _caret_hint(old: str, new: str) -> str: + """Build a ``^`` marker line highlighting changed spans in *new*. + + Uses SequenceMatcher to find which parts of *new* differ from *old* + and places ``^`` characters under them (aligned with the ``+ `` prefix). + Returns an empty string when the whole line changed (carets wouldn't help). + """ + matcher = SequenceMatcher(None, old, new) + carets = [" "] * len(new) + changed_count = 0 + for tag, _i1, _i2, j1, j2 in matcher.get_opcodes(): + if tag != "equal": + for j in range(j1, j2): + carets[j] = "^" + changed_count += j2 - j1 + # If most of the string changed, carets are just noise. + if changed_count > len(new) * 0.6: + return "" + return "".join(carets).rstrip() class Comparator: - """Cross-reference two primer JSON outputs and iterate over differences.""" + """Cross-reference two primer JSON outputs and iterate over differences. + + Yields ``PackageDiff`` entries carrying missing, new and changed messages + for each package that has at least one difference. *changed* contains + pairs of ``(old, new)`` messages that are the same diagnostic but with + altered details (line, message text, etc.). + """ def __init__(self, main_data: PackageMessages, pr_data: PackageMessages) -> None: self._main_data = main_data @@ -58,24 +177,31 @@ def __iter__(self) -> Iterator[PackageDiff]: main_data = self._main_data pr_data = self._pr_data - missing_messages: PackageMessages = {} for package, data in main_data.items(): - package_missing_messages: list[JSONMessage] = [] + # First pass: exact-match removal. + pr_messages = list(pr_data[package]["messages"]) + residual_old: list[JSONMessage] = [] for message in data["messages"]: try: - pr_data[package]["messages"].remove(message) + pr_messages.remove(message) except ValueError: - package_missing_messages.append(message) - missing_messages[package] = PackageData( - commit=pr_data[package]["commit"], - messages=package_missing_messages, + residual_old.append(message) + + # Second pass: pair residuals by position to detect *changed* + # messages (same diagnostic, different line or text). + paired, final_missing, final_new = _match_residuals( + residual_old, pr_messages ) - for package, pkg_missing in missing_messages.items(): - new_messages = pr_data[package] - if not pkg_missing["messages"] and not new_messages["messages"]: + if not final_missing and not final_new and not paired: continue - yield PackageDiff(package, pkg_missing, new_messages) + commit = pr_data[package]["commit"] + yield PackageDiff( + package, + missing=PackageData(commit=commit, messages=final_missing), + new=PackageData(commit=commit, messages=final_new), + changed=paired, + ) @staticmethod def _load_json(file_path: Path | str) -> PackageMessages: diff --git a/pylint/testutils/_primer/primer_compare_command.py b/pylint/testutils/_primer/primer_compare_command.py index e1c02300ad..406e41035e 100644 --- a/pylint/testutils/_primer/primer_compare_command.py +++ b/pylint/testutils/_primer/primer_compare_command.py @@ -5,8 +5,13 @@ from pathlib import PurePosixPath -from pylint.testutils._primer.comparator import Comparator -from pylint.testutils._primer.primer_command import PackageData, PrimerCommand +from pylint.reporters.json_reporter import JSONMessage +from pylint.testutils._primer.comparator import ( + Comparator, + PackageDiff, + message_diff, +) +from pylint.testutils._primer.primer_command import PrimerCommand MAX_GITHUB_COMMENT_LENGTH = 65536 @@ -25,10 +30,7 @@ def _create_comment(self, comparator: Comparator) -> str: for diff in comparator: if len(comment) >= MAX_GITHUB_COMMENT_LENGTH: break - package = diff.package - comment += f"\n**Effect on [{package}]({self.packages[package].url}):**\n\n" - comment += self._format_new_messages(package, diff.new) - comment += self._format_missing_messages(package, diff.missing) + comment += self._create_comment_for_package(diff) comment = ( f"🤖 **Effect of this PR on checked open source code:** 🤖\n\n{comment}" if comment @@ -39,27 +41,47 @@ def _create_comment(self, comparator: Comparator) -> str: ) return self._truncate_comment(comment) - def _format_new_messages(self, package: str, new_messages: PackageData) -> str: - comment = "" + def _create_comment_for_package(self, diff: PackageDiff) -> str: + package = diff.package + url = self.packages[package].url + clone_dir = self.packages[package].clone_directory + commit = diff.new["commit"] + + assert not url.endswith( + ".git" + ), "You don't need the .git at the end of the github url." + + def _source_link(msg: JSONMessage) -> str: + filepath = str(PurePosixPath(msg["path"]).relative_to(clone_dir)) + return f"{url}/blob/{commit}/{filepath}#L{msg['line']}" + + comment = f"\n**Effect on [{package}]({url}):**\n\n" + + # Changed messages + if diff.changed: + print("Changed:") + comment += "The following messages have been changed:\n\n
\n\n" + for count, (old, new) in enumerate(diff.changed, 1): + comment += ( + f"{count}) [{new['symbol']}]({_source_link(new)}):\n" + f"{message_diff(old, new)}\n" + ) + print(new) + comment += "
\n\n" + + # New messages count = 1 astroid_errors = 0 new_non_astroid_messages = "" - if new_messages["messages"]: + if diff.new["messages"]: print("Now emitted:") - 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. + for message in diff.new["messages"]: if message["symbol"] == "astroid-error": astroid_errors += 1 else: new_non_astroid_messages += ( f"{count}) {message['symbol']}:\n*{message['message']}*\n" - f"{self.packages[package].url}/blob/{new_messages['commit']}/{filepath}#L{message['line']}\n" + f"{_source_link(message)}\n" ) print(message) count += 1 @@ -75,33 +97,20 @@ def _format_new_messages(self, package: str, new_messages: PackageData) -> str: + new_non_astroid_messages + "\n\n" ) - return comment - def _format_missing_messages( - self, package: str, missing_messages: PackageData - ) -> str: - comment = "" + # Missing messages count = 1 - if missing_messages["messages"]: + if diff.missing["messages"]: comment += "The following messages are no longer emitted:\n\n
\n\n" print("No longer emitted:") - for message in missing_messages["messages"]: - comment += f"{count}) {message['symbol']}:\n*{message['message']}*\n" - 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." + for message in diff.missing["messages"]: comment += ( - f"{self.packages[package].url}" - f"/blob/{missing_messages['commit']}/{filepath}#L{message['line']}\n" + f"{count}) {message['symbol']}:\n*{message['message']}*\n" + f"{_source_link(message)}\n" ) count += 1 print(message) - if missing_messages["messages"]: + if diff.missing["messages"]: comment += "
\n\n" return comment diff --git a/tests/testutils/_primer/batched_cases/expected_comparator.json b/tests/testutils/_primer/batched_cases/expected_comparator.json index e3a42bfd31..3be623243d 100644 --- a/tests/testutils/_primer/batched_cases/expected_comparator.json +++ b/tests/testutils/_primer/batched_cases/expected_comparator.json @@ -1 +1 @@ -[{ "package": "astroid", "missing": 2, "new": 0 }] +[{ "package": "astroid", "missing": 2, "new": 0, "changed": 0 }] diff --git a/tests/testutils/_primer/cases/confidence_changed/expected.txt b/tests/testutils/_primer/cases/confidence_changed/expected.txt index 8555240d13..bb0e8e2898 100644 --- a/tests/testutils/_primer/cases/confidence_changed/expected.txt +++ b/tests/testutils/_primer/cases/confidence_changed/expected.txt @@ -3,22 +3,12 @@ **Effect on [astroid](https://github.com/pylint-dev/astroid):** -The following messages are now emitted: +The following messages have been changed:
-1) dangerous-default-value: -*Dangerous default value [] as argument* -https://github.com/pylint-dev/astroid/blob/123456789abcdef/astroid/inference.py#L42 -
- -The following messages are no longer emitted: - -
- -1) dangerous-default-value: -*Dangerous default value [] as argument* -https://github.com/pylint-dev/astroid/blob/123456789abcdef/astroid/inference.py#L42 +1) [dangerous-default-value](https://github.com/pylint-dev/astroid/blob/123456789abcdef/astroid/inference.py#L42): +confidence: `UNDEFINED` → `HIGH`
*This comment was generated for commit v2.14.2* diff --git a/tests/testutils/_primer/cases/confidence_changed/expected_comparator.json b/tests/testutils/_primer/cases/confidence_changed/expected_comparator.json index d826f8390a..47824cb2e1 100644 --- a/tests/testutils/_primer/cases/confidence_changed/expected_comparator.json +++ b/tests/testutils/_primer/cases/confidence_changed/expected_comparator.json @@ -1 +1 @@ -[{ "package": "astroid", "missing": 1, "new": 1 }] +[{ "package": "astroid", "missing": 0, "new": 0, "changed": 1 }] diff --git a/tests/testutils/_primer/cases/line_moved/expected.txt b/tests/testutils/_primer/cases/line_moved/expected.txt index 27266ea0b8..ca0b3aaa76 100644 --- a/tests/testutils/_primer/cases/line_moved/expected.txt +++ b/tests/testutils/_primer/cases/line_moved/expected.txt @@ -3,22 +3,12 @@ **Effect on [astroid](https://github.com/pylint-dev/astroid):** -The following messages are now emitted: +The following messages have been changed:
-1) too-complex: -*'my_function' is too complex. The McCabe rating is 18* -https://github.com/pylint-dev/astroid/blob/123456789abcdef/astroid/node_classes.py#L103 -
- -The following messages are no longer emitted: - -
- -1) too-complex: -*'my_function' is too complex. The McCabe rating is 18* -https://github.com/pylint-dev/astroid/blob/123456789abcdef/astroid/node_classes.py#L100 +1) [too-complex](https://github.com/pylint-dev/astroid/blob/123456789abcdef/astroid/node_classes.py#L103): +Was raised from `100:0` to `100:15`, now from `103:0` to `103:15`.
*This comment was generated for commit v2.14.2* diff --git a/tests/testutils/_primer/cases/line_moved/expected_comparator.json b/tests/testutils/_primer/cases/line_moved/expected_comparator.json index d826f8390a..47824cb2e1 100644 --- a/tests/testutils/_primer/cases/line_moved/expected_comparator.json +++ b/tests/testutils/_primer/cases/line_moved/expected_comparator.json @@ -1 +1 @@ -[{ "package": "astroid", "missing": 1, "new": 1 }] +[{ "package": "astroid", "missing": 0, "new": 0, "changed": 1 }] diff --git a/tests/testutils/_primer/cases/message_changed/expected.txt b/tests/testutils/_primer/cases/message_changed/expected.txt index a99896696e..780f055938 100644 --- a/tests/testutils/_primer/cases/message_changed/expected.txt +++ b/tests/testutils/_primer/cases/message_changed/expected.txt @@ -3,22 +3,16 @@ **Effect on [astroid](https://github.com/pylint-dev/astroid):** -The following messages are now emitted: +The following messages have been changed:
-1) locally-disabled: -*Locally disabling redefined-builtin [we added some text in the message] (W0622)* -https://github.com/pylint-dev/astroid/blob/123456789abcdef/astroid/__init__.py#L91 -
- -The following messages are no longer emitted: - -
- -1) locally-disabled: -*Locally disabling redefined-builtin (W0622)* -https://github.com/pylint-dev/astroid/blob/123456789abcdef/astroid/__init__.py#L91 +1) [locally-disabled](https://github.com/pylint-dev/astroid/blob/123456789abcdef/astroid/__init__.py#L91): +```diff +- Locally disabling redefined-builtin (W0622) ++ Locally disabling redefined-builtin [we added some text in the message] (W0622) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +```
*This comment was generated for commit v2.14.2* diff --git a/tests/testutils/_primer/cases/message_changed/expected_comparator.json b/tests/testutils/_primer/cases/message_changed/expected_comparator.json index d826f8390a..47824cb2e1 100644 --- a/tests/testutils/_primer/cases/message_changed/expected_comparator.json +++ b/tests/testutils/_primer/cases/message_changed/expected_comparator.json @@ -1 +1 @@ -[{ "package": "astroid", "missing": 1, "new": 1 }] +[{ "package": "astroid", "missing": 0, "new": 0, "changed": 1 }] diff --git a/tests/testutils/_primer/cases/message_changed/expected_truncated.txt b/tests/testutils/_primer/cases/message_changed/expected_truncated.txt index 44926e4485..d3943d8a77 100644 --- a/tests/testutils/_primer/cases/message_changed/expected_truncated.txt +++ b/tests/testutils/_primer/cases/message_changed/expected_truncated.txt @@ -3,12 +3,14 @@ **Effect on [astroid](https://github.com/pylint-dev/astroid):** -The following messages are now emitted: +The following messages have been changed:
-1) locally-disabled: -*Locally disabling redefined-builtin [we added some text in the message]... +1) [locally-disabled](https://github.com/pylint-dev/astroid/blob/123456789abcdef/astroid/__init__.py#L91): +```diff +- Locally disabling redefined-builtin (W0622) ++ Locally disabling...
*This comment was truncated because GitHub allows only 525 characters in a comment.* diff --git a/tests/testutils/_primer/cases/message_changed/expected_truncated_in_details.txt b/tests/testutils/_primer/cases/message_changed/expected_truncated_in_details.txt index c8406b21f8..f3ae7aa165 100644 --- a/tests/testutils/_primer/cases/message_changed/expected_truncated_in_details.txt +++ b/tests/testutils/_primer/cases/message_changed/expected_truncated_in_details.txt @@ -3,12 +3,11 @@ **Effect on [astroid](https://github.com/pylint-dev/astroid):** -The following messages are now emitted: +The following messages have been changed:
-1) locally-disabled: -*Locally disabling redefined-builtin [we added some text in the... +1)...
*This comment was truncated because GitHub allows only 420 characters in a comment.* diff --git a/tests/testutils/_primer/cases/message_changed_line/expected.txt b/tests/testutils/_primer/cases/message_changed_line/expected.txt index 959804eb5a..c9646807e5 100644 --- a/tests/testutils/_primer/cases/message_changed_line/expected.txt +++ b/tests/testutils/_primer/cases/message_changed_line/expected.txt @@ -3,22 +3,18 @@ **Effect on [astroid](https://github.com/pylint-dev/astroid):** -The following messages are now emitted: +The following messages have been changed:
-1) too-complex: -*'my_function' is too complex. The McCabe rating is 17* -https://github.com/pylint-dev/astroid/blob/123456789abcdef/astroid/node_classes.py#L103 -
- -The following messages are no longer emitted: - -
+1) [too-complex](https://github.com/pylint-dev/astroid/blob/123456789abcdef/astroid/node_classes.py#L103): +Was raised from `100:0` to `100:15`, now from `103:0` to `103:15`. -1) too-complex: -*'my_function' is too complex. The McCabe rating is 18* -https://github.com/pylint-dev/astroid/blob/123456789abcdef/astroid/node_classes.py#L100 +```diff +- 'my_function' is too complex. The McCabe rating is 18 ++ 'my_function' is too complex. The McCabe rating is 17 + ^ +```
*This comment was generated for commit v2.14.2* diff --git a/tests/testutils/_primer/cases/message_changed_line/expected_comparator.json b/tests/testutils/_primer/cases/message_changed_line/expected_comparator.json index d826f8390a..47824cb2e1 100644 --- a/tests/testutils/_primer/cases/message_changed_line/expected_comparator.json +++ b/tests/testutils/_primer/cases/message_changed_line/expected_comparator.json @@ -1 +1 @@ -[{ "package": "astroid", "missing": 1, "new": 1 }] +[{ "package": "astroid", "missing": 0, "new": 0, "changed": 1 }] diff --git a/tests/testutils/_primer/cases/mixed_changes/expected.txt b/tests/testutils/_primer/cases/mixed_changes/expected.txt index 9590f932d6..2f735a8cb5 100644 --- a/tests/testutils/_primer/cases/mixed_changes/expected.txt +++ b/tests/testutils/_primer/cases/mixed_changes/expected.txt @@ -3,28 +3,28 @@ **Effect on [astroid](https://github.com/pylint-dev/astroid):** -The following messages are now emitted: +The following messages have been changed:
-1) locally-disabled: -*Locally disabling redefined-builtin [we added some text in the message] (W0622)* -https://github.com/pylint-dev/astroid/blob/123456789abcdef/astroid/__init__.py#L91 -2) too-complex: -*'my_function' is too complex. The McCabe rating is 17* -https://github.com/pylint-dev/astroid/blob/123456789abcdef/astroid/node_classes.py#L105 -
- -The following messages are no longer emitted: - -
- -1) locally-disabled: -*Locally disabling redefined-builtin (W0622)* -https://github.com/pylint-dev/astroid/blob/123456789abcdef/astroid/__init__.py#L91 -2) too-complex: -*'my_function' is too complex. The McCabe rating is 18* -https://github.com/pylint-dev/astroid/blob/123456789abcdef/astroid/node_classes.py#L100 +1) [locally-disabled](https://github.com/pylint-dev/astroid/blob/123456789abcdef/astroid/__init__.py#L91): +```diff +- Locally disabling redefined-builtin (W0622) ++ Locally disabling redefined-builtin [we added some text in the message] (W0622) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +``` +2) [too-complex](https://github.com/pylint-dev/astroid/blob/123456789abcdef/astroid/node_classes.py#L105): +Was raised from `100:0` to `100:15`, now from `105:0` to `105:15`. + +confidence: `UNDEFINED` → `HIGH` + +```diff +- 'my_function' is too complex. The McCabe rating is 18 ++ 'my_function' is too complex. The McCabe rating is 17 + ^ +``` +3) [unknown-option-value](https://github.com/pylint-dev/astroid/blob/123456789abcdef/astroid/__init__.py#L91): +confidence: `UNDEFINED` → `HIGH`
*This comment was generated for commit v2.14.2* diff --git a/tests/testutils/_primer/cases/mixed_changes/expected_comparator.json b/tests/testutils/_primer/cases/mixed_changes/expected_comparator.json index a0e7f6f710..6e33a850d4 100644 --- a/tests/testutils/_primer/cases/mixed_changes/expected_comparator.json +++ b/tests/testutils/_primer/cases/mixed_changes/expected_comparator.json @@ -1 +1 @@ -[{ "package": "astroid", "missing": 2, "new": 2 }] +[{ "package": "astroid", "missing": 0, "new": 0, "changed": 3 }] diff --git a/tests/testutils/_primer/cases/mixed_changes/pr.json b/tests/testutils/_primer/cases/mixed_changes/pr.json index 87d7e44b04..fbadd2d9fe 100644 --- a/tests/testutils/_primer/cases/mixed_changes/pr.json +++ b/tests/testutils/_primer/cases/mixed_changes/pr.json @@ -29,7 +29,7 @@ "symbol": "too-complex", "message": "'my_function' is too complex. The McCabe rating is 17", "messageId": "R1260", - "confidence": "UNDEFINED", + "confidence": "HIGH", "absolutePath": "tests/.pylint_primer_tests/pylint-dev/astroid/astroid/node_classes.py" }, { @@ -44,7 +44,7 @@ "symbol": "unknown-option-value", "message": "Unknown option value for 'disable', expected a valid pylint message and got 'Ellipsis'", "messageId": "W0012", - "confidence": "UNDEFINED", + "confidence": "HIGH", "absolutePath": "tests/.pylint_primer_tests/pylint-dev/astroid/astroid/__init__.py" } ] diff --git a/tests/testutils/_primer/cases/multi_package/expected.txt b/tests/testutils/_primer/cases/multi_package/expected.txt index 46f9611eb7..9f4a1f9c13 100644 --- a/tests/testutils/_primer/cases/multi_package/expected.txt +++ b/tests/testutils/_primer/cases/multi_package/expected.txt @@ -24,7 +24,7 @@ https://github.com/pylint-dev/astroid/blob/123456789abcdef/astroid/node_classes. **Effect on [astropy](https://github.com/astropy/astropy):** -The following messages are now emitted: +😞 The following false positives have been introduced:
diff --git a/tests/testutils/_primer/cases/multi_package/expected_comparator.json b/tests/testutils/_primer/cases/multi_package/expected_comparator.json index 456a50f703..38f460088e 100644 --- a/tests/testutils/_primer/cases/multi_package/expected_comparator.json +++ b/tests/testutils/_primer/cases/multi_package/expected_comparator.json @@ -1,4 +1,4 @@ [ - { "package": "astroid", "missing": 1, "new": 1 }, - { "package": "astropy", "missing": 1, "new": 1 } + { "package": "astroid", "missing": 0, "new": 0, "changed": 1 }, + { "package": "astropy", "missing": 1, "new": 1, "changed": 0 } ] diff --git a/tests/testutils/_primer/cases/new_message/expected_comparator.json b/tests/testutils/_primer/cases/new_message/expected_comparator.json index e436268fa3..1d944b5bb8 100644 --- a/tests/testutils/_primer/cases/new_message/expected_comparator.json +++ b/tests/testutils/_primer/cases/new_message/expected_comparator.json @@ -1 +1 @@ -[{ "package": "astroid", "missing": 0, "new": 1 }] +[{ "package": "astroid", "missing": 0, "new": 1, "changed": 0 }] diff --git a/tests/testutils/_primer/cases/removed_message/expected_comparator.json b/tests/testutils/_primer/cases/removed_message/expected_comparator.json index cd879eeea8..723706ace4 100644 --- a/tests/testutils/_primer/cases/removed_message/expected_comparator.json +++ b/tests/testutils/_primer/cases/removed_message/expected_comparator.json @@ -1 +1 @@ -[{ "package": "astroid", "missing": 1, "new": 0 }] +[{ "package": "astroid", "missing": 1, "new": 0, "changed": 0 }] diff --git a/tests/testutils/_primer/cases/type_changed/expected.txt b/tests/testutils/_primer/cases/type_changed/expected.txt index d83b197c08..da46e99bb1 100644 --- a/tests/testutils/_primer/cases/type_changed/expected.txt +++ b/tests/testutils/_primer/cases/type_changed/expected.txt @@ -3,22 +3,12 @@ **Effect on [astroid](https://github.com/pylint-dev/astroid):** -The following messages are now emitted: +The following messages have been changed:
-1) line-too-long: -*Line too long (120/100)* -https://github.com/pylint-dev/astroid/blob/123456789abcdef/astroid/node_classes.py#L50 -
- -The following messages are no longer emitted: - -
- -1) line-too-long: -*Line too long (120/100)* -https://github.com/pylint-dev/astroid/blob/123456789abcdef/astroid/node_classes.py#L50 +1) [line-too-long](https://github.com/pylint-dev/astroid/blob/123456789abcdef/astroid/node_classes.py#L50): +type: `convention` → `warning`
*This comment was generated for commit v2.14.2* diff --git a/tests/testutils/_primer/cases/type_changed/expected_comparator.json b/tests/testutils/_primer/cases/type_changed/expected_comparator.json index d826f8390a..47824cb2e1 100644 --- a/tests/testutils/_primer/cases/type_changed/expected_comparator.json +++ b/tests/testutils/_primer/cases/type_changed/expected_comparator.json @@ -1 +1 @@ -[{ "package": "astroid", "missing": 1, "new": 1 }] +[{ "package": "astroid", "missing": 0, "new": 0, "changed": 1 }] diff --git a/tests/testutils/_primer/cases/useless_suppression/expected_comparator.json b/tests/testutils/_primer/cases/useless_suppression/expected_comparator.json index b02271a012..68ea0d61b5 100644 --- a/tests/testutils/_primer/cases/useless_suppression/expected_comparator.json +++ b/tests/testutils/_primer/cases/useless_suppression/expected_comparator.json @@ -1 +1 @@ -[{ "package": "astropy", "missing": 0, "new": 2 }] +[{ "package": "astropy", "missing": 0, "new": 2, "changed": 0 }] diff --git a/tests/testutils/_primer/test_comparator.py b/tests/testutils/_primer/test_comparator.py index 212af59e61..94e8d035a0 100644 --- a/tests/testutils/_primer/test_comparator.py +++ b/tests/testutils/_primer/test_comparator.py @@ -22,10 +22,11 @@ def test_comparator(directory: Path) -> None: expected = json.loads((directory / "expected_comparator.json").read_text("utf-8")) results = list(comparator) assert len(results) == len(expected) - for (package, missing, new), exp in zip(results, expected): + for (package, missing, new, changed), exp in zip(results, expected): assert package == exp["package"] assert len(missing["messages"]) == exp["missing"] assert len(new["messages"]) == exp["new"] + assert len(changed) == exp["changed"] def test_comparator_batched() -> None: @@ -38,7 +39,8 @@ def test_comparator_batched() -> None: expected = json.loads((fixture / "expected_comparator.json").read_text("utf-8")) results = list(comparator) assert len(results) == len(expected) - for (package, missing, new), exp in zip(results, expected): + for (package, missing, new, changed), exp in zip(results, expected): assert package == exp["package"] assert len(missing["messages"]) == exp["missing"] assert len(new["messages"]) == exp["new"] + assert len(changed) == exp["changed"] From d1dda8b8704be8233a0ddf969f005d4f7e6db5f9 Mon Sep 17 00:00:00 2001 From: Pierre Sassoulas Date: Thu, 2 Apr 2026 22:51:05 +0200 Subject: [PATCH 2/8] [primer] Classify new messages into false-positive categories MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit New messages are now sorted into subcategories: - useless-suppression → "fixed false positives" (🎉) - locally-disabled → "introduced false positives" (😞) - astroid-error → counted separately - everything else → "now emitted" Extract _ClassifiedMessages and _format_messages helpers, and use _details_section for consistent
blocks with the blank line required for GitHub markdown rendering. --- .../_primer/primer_compare_command.py | 145 ++++++++++++------ .../_primer/batched_cases/expected.txt | 2 +- .../cases/confidence_changed/expected.txt | 2 +- .../_primer/cases/line_moved/expected.txt | 2 +- .../cases/message_changed/expected.txt | 2 +- .../message_changed/expected_truncated.txt | 7 +- .../expected_truncated_in_details.txt | 4 +- .../cases/message_changed_line/expected.txt | 2 +- .../_primer/cases/mixed_changes/expected.txt | 2 +- .../_primer/cases/multi_package/expected.txt | 24 ++- .../_primer/cases/new_message/expected.txt | 2 +- .../cases/removed_message/expected.txt | 2 +- .../_primer/cases/type_changed/expected.txt | 2 +- .../cases/useless_suppression/expected.txt | 14 +- tests/testutils/_primer/test_primer.py | 4 +- 15 files changed, 136 insertions(+), 80 deletions(-) diff --git a/pylint/testutils/_primer/primer_compare_command.py b/pylint/testutils/_primer/primer_compare_command.py index 406e41035e..f5df3fc3f0 100644 --- a/pylint/testutils/_primer/primer_compare_command.py +++ b/pylint/testutils/_primer/primer_compare_command.py @@ -3,10 +3,13 @@ # 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 JSONMessage from pylint.testutils._primer.comparator import ( + ChangedMessage, Comparator, PackageDiff, message_diff, @@ -14,6 +17,70 @@ from pylint.testutils._primer.primer_command import PrimerCommand MAX_GITHUB_COMMENT_LENGTH = 65536 +USELESS_SUPPRESSION_RE = re.compile(r"Useless suppression of '(.+)'") + + +def _format_messages( + messages: list[JSONMessage], + source_link: Callable[[JSONMessage], str], +) -> str: + """Format a list of messages as a numbered body for a ``
`` block.""" + body = "" + for count, msg in enumerate(messages, 1): + body += ( + f"{count}) {msg['symbol']}:\n*{msg['message']}*\n" f"{source_link(msg)}\n" + ) + return body + + +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[JSONMessage], + missing_messages: list[JSONMessage], + changed_messages: list[ChangedMessage], + source_link: Callable[[JSONMessage], str], + ) -> None: + astroid_errors = 0 + new_fp: list[JSONMessage] = [] + fixed_fp: list[JSONMessage] = [] + other_new: list[JSONMessage] = [] + 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) + + self.astroid_errors = astroid_errors + self.fixed_false_positives = _format_messages(fixed_fp, source_link) + self.new_false_positives = _format_messages(new_fp, source_link) + self.new_messages = _format_messages(other_new, source_link) + self.missing_messages = _format_messages(missing_messages, source_link) + + 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): @@ -55,63 +122,51 @@ def _source_link(msg: JSONMessage) -> str: filepath = str(PurePosixPath(msg["path"]).relative_to(clone_dir)) return f"{url}/blob/{commit}/{filepath}#L{msg['line']}" + def _details_section(title: str, body: str) -> str: + # Blank line after
required for GitHub markdown rendering. + return f"{title}\n\n
\n\n{body}
\n\n" + + classified = _ClassifiedMessages( + diff.new["messages"], diff.missing["messages"], diff.changed, _source_link + ) comment = f"\n**Effect on [{package}]({url}):**\n\n" - # Changed messages if diff.changed: print("Changed:") - comment += "The following messages have been changed:\n\n
\n\n" - for count, (old, new) in enumerate(diff.changed, 1): - comment += ( - f"{count}) [{new['symbol']}]({_source_link(new)}):\n" - f"{message_diff(old, new)}\n" - ) + for _, new in diff.changed: print(new) - comment += "
\n\n" - - # New messages - count = 1 - astroid_errors = 0 - new_non_astroid_messages = "" if diff.new["messages"]: print("Now emitted:") - for message in diff.new["messages"]: - if message["symbol"] == "astroid-error": - astroid_errors += 1 - else: - new_non_astroid_messages += ( - f"{count}) {message['symbol']}:\n*{message['message']}*\n" - f"{_source_link(message)}\n" - ) - print(message) - count += 1 - - if astroid_errors: + for msg in diff.new["messages"]: + print(msg) + if diff.missing["messages"]: + print("No longer emitted:") + for msg in diff.missing["messages"]: + print(msg) + + if classified.changed_messages: + comment += _details_section( + "Changed messages:", classified.changed_messages + ) + 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: - comment += ( - "The following messages are now emitted:\n\n
\n\n" - + new_non_astroid_messages - + "
\n\n" + if classified.fixed_false_positives: + comment += _details_section( + "🎉 Fixed false positives:", classified.fixed_false_positives ) - - # Missing messages - count = 1 - if diff.missing["messages"]: - comment += "The following messages are no longer emitted:\n\n
\n\n" - print("No longer emitted:") - for message in diff.missing["messages"]: - comment += ( - f"{count}) {message['symbol']}:\n*{message['message']}*\n" - f"{_source_link(message)}\n" + if classified.new_false_positives: + comment += _details_section( + "😞 New false positives:", classified.new_false_positives + ) + if classified.new_messages: + comment += _details_section("New messages:", classified.new_messages) + if classified.missing_messages: + comment += _details_section( + "Removed messages:", classified.missing_messages ) - count += 1 - print(message) - if diff.missing["messages"]: - comment += "
\n\n" return comment def _truncate_comment(self, comment: str) -> str: diff --git a/tests/testutils/_primer/batched_cases/expected.txt b/tests/testutils/_primer/batched_cases/expected.txt index 2c112f6e75..9427ea15e6 100644 --- a/tests/testutils/_primer/batched_cases/expected.txt +++ b/tests/testutils/_primer/batched_cases/expected.txt @@ -3,7 +3,7 @@ **Effect on [astroid](https://github.com/pylint-dev/astroid):** -The following messages are no longer emitted: +Removed messages:
diff --git a/tests/testutils/_primer/cases/confidence_changed/expected.txt b/tests/testutils/_primer/cases/confidence_changed/expected.txt index bb0e8e2898..717bb004f8 100644 --- a/tests/testutils/_primer/cases/confidence_changed/expected.txt +++ b/tests/testutils/_primer/cases/confidence_changed/expected.txt @@ -3,7 +3,7 @@ **Effect on [astroid](https://github.com/pylint-dev/astroid):** -The following messages have been changed: +Changed messages:
diff --git a/tests/testutils/_primer/cases/line_moved/expected.txt b/tests/testutils/_primer/cases/line_moved/expected.txt index ca0b3aaa76..03d388dea3 100644 --- a/tests/testutils/_primer/cases/line_moved/expected.txt +++ b/tests/testutils/_primer/cases/line_moved/expected.txt @@ -3,7 +3,7 @@ **Effect on [astroid](https://github.com/pylint-dev/astroid):** -The following messages have been changed: +Changed messages:
diff --git a/tests/testutils/_primer/cases/message_changed/expected.txt b/tests/testutils/_primer/cases/message_changed/expected.txt index 780f055938..cb4812421a 100644 --- a/tests/testutils/_primer/cases/message_changed/expected.txt +++ b/tests/testutils/_primer/cases/message_changed/expected.txt @@ -3,7 +3,7 @@ **Effect on [astroid](https://github.com/pylint-dev/astroid):** -The following messages have been changed: +Changed messages:
diff --git a/tests/testutils/_primer/cases/message_changed/expected_truncated.txt b/tests/testutils/_primer/cases/message_changed/expected_truncated.txt index d3943d8a77..55d70bb96f 100644 --- a/tests/testutils/_primer/cases/message_changed/expected_truncated.txt +++ b/tests/testutils/_primer/cases/message_changed/expected_truncated.txt @@ -3,16 +3,15 @@ **Effect on [astroid](https://github.com/pylint-dev/astroid):** -The following messages have been changed: +Changed messages:
1) [locally-disabled](https://github.com/pylint-dev/astroid/blob/123456789abcdef/astroid/__init__.py#L91): ```diff -- Locally disabling redefined-builtin (W0622) -+ Locally disabling... +- Locally disabling...
-*This comment was truncated because GitHub allows only 525 characters in a comment.* +*This comment was truncated because GitHub allows only 450 characters in a comment.* *This comment was generated for commit v2.14.2* diff --git a/tests/testutils/_primer/cases/message_changed/expected_truncated_in_details.txt b/tests/testutils/_primer/cases/message_changed/expected_truncated_in_details.txt index f3ae7aa165..a285996bf7 100644 --- a/tests/testutils/_primer/cases/message_changed/expected_truncated_in_details.txt +++ b/tests/testutils/_primer/cases/message_changed/expected_truncated_in_details.txt @@ -3,13 +3,13 @@ **Effect on [astroid](https://github.com/pylint-dev/astroid):** -The following messages have been changed: +Changed messages:
1)...
-*This comment was truncated because GitHub allows only 420 characters in a comment.* +*This comment was truncated because GitHub allows only 400 characters in a comment.* *This comment was generated for commit v2.14.2* diff --git a/tests/testutils/_primer/cases/message_changed_line/expected.txt b/tests/testutils/_primer/cases/message_changed_line/expected.txt index c9646807e5..aab58798d7 100644 --- a/tests/testutils/_primer/cases/message_changed_line/expected.txt +++ b/tests/testutils/_primer/cases/message_changed_line/expected.txt @@ -3,7 +3,7 @@ **Effect on [astroid](https://github.com/pylint-dev/astroid):** -The following messages have been changed: +Changed messages:
diff --git a/tests/testutils/_primer/cases/mixed_changes/expected.txt b/tests/testutils/_primer/cases/mixed_changes/expected.txt index 2f735a8cb5..b40a50c3a7 100644 --- a/tests/testutils/_primer/cases/mixed_changes/expected.txt +++ b/tests/testutils/_primer/cases/mixed_changes/expected.txt @@ -3,7 +3,7 @@ **Effect on [astroid](https://github.com/pylint-dev/astroid):** -The following messages have been changed: +Changed messages:
diff --git a/tests/testutils/_primer/cases/multi_package/expected.txt b/tests/testutils/_primer/cases/multi_package/expected.txt index 9f4a1f9c13..a4f6ea541f 100644 --- a/tests/testutils/_primer/cases/multi_package/expected.txt +++ b/tests/testutils/_primer/cases/multi_package/expected.txt @@ -3,28 +3,24 @@ **Effect on [astroid](https://github.com/pylint-dev/astroid):** -The following messages are now emitted: +Changed messages:
-1) too-complex: -*'my_function' is too complex. The McCabe rating is 17* -https://github.com/pylint-dev/astroid/blob/123456789abcdef/astroid/node_classes.py#L105 -
- -The following messages are no longer emitted: - -
+1) [too-complex](https://github.com/pylint-dev/astroid/blob/123456789abcdef/astroid/node_classes.py#L105): +Was raised from `100:0` to `100:15`, now from `105:0` to `105:15`. -1) too-complex: -*'my_function' is too complex. The McCabe rating is 18* -https://github.com/pylint-dev/astroid/blob/123456789abcdef/astroid/node_classes.py#L100 +```diff +- 'my_function' is too complex. The McCabe rating is 18 ++ 'my_function' is too complex. The McCabe rating is 17 + ^ +```
**Effect on [astropy](https://github.com/astropy/astropy):** -😞 The following false positives have been introduced: +😞 New false positives:
@@ -33,7 +29,7 @@ https://github.com/pylint-dev/astroid/blob/123456789abcdef/astroid/node_classes. https://github.com/astropy/astropy/blob/1fb40bc1f22f176254ef583065aa155f53f3b414/astropy/coordinates/angles/angle_parsetab.py#L14
-The following messages are no longer emitted: +Removed messages:
diff --git a/tests/testutils/_primer/cases/new_message/expected.txt b/tests/testutils/_primer/cases/new_message/expected.txt index 425df41d6a..e813dd0a86 100644 --- a/tests/testutils/_primer/cases/new_message/expected.txt +++ b/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: +😞 New false positives:
diff --git a/tests/testutils/_primer/cases/removed_message/expected.txt b/tests/testutils/_primer/cases/removed_message/expected.txt index 88af536d71..0200400648 100644 --- a/tests/testutils/_primer/cases/removed_message/expected.txt +++ b/tests/testutils/_primer/cases/removed_message/expected.txt @@ -3,7 +3,7 @@ **Effect on [astroid](https://github.com/pylint-dev/astroid):** -The following messages are no longer emitted: +Removed messages:
diff --git a/tests/testutils/_primer/cases/type_changed/expected.txt b/tests/testutils/_primer/cases/type_changed/expected.txt index da46e99bb1..64423538a0 100644 --- a/tests/testutils/_primer/cases/type_changed/expected.txt +++ b/tests/testutils/_primer/cases/type_changed/expected.txt @@ -3,7 +3,7 @@ **Effect on [astroid](https://github.com/pylint-dev/astroid):** -The following messages have been changed: +Changed messages:
diff --git a/tests/testutils/_primer/cases/useless_suppression/expected.txt b/tests/testutils/_primer/cases/useless_suppression/expected.txt index 8f56c44551..a813b98174 100644 --- a/tests/testutils/_primer/cases/useless_suppression/expected.txt +++ b/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: +🎉 Fixed false positives: + +
+ +1) useless-suppression: +*Useless suppression of 'looping-through-iterator'* +https://github.com/astropy/astropy/blob/1fb40bc1f22f176254ef583065aa155f53f3b414/astropy/coordinates/angles/angle_parsetab.py#L14 +
+ +😞 New false positives:
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
*This comment was generated for commit v2.14.2* diff --git a/tests/testutils/_primer/test_primer.py b/tests/testutils/_primer/test_primer.py index d639a27c19..d443cde693 100644 --- a/tests/testutils/_primer/test_primer.py +++ b/tests/testutils/_primer/test_primer.py @@ -77,7 +77,7 @@ def test_compare_batched(self) -> None: def test_truncated_compare(self) -> None: """Test for the truncation of comments that are too long.""" - max_comment_length = 525 + max_comment_length = 450 directory = CASES_PATH / "message_changed" with patch( "pylint.testutils._primer.primer_compare_command.MAX_GITHUB_COMMENT_LENGTH", @@ -107,7 +107,7 @@ def test_truncated_compare_stops_iterating_packages(self) -> None: def test_truncated_compare_in_details(self) -> None: """Test for the truncation of comments that are too long inside details.""" - max_comment_length = 420 + max_comment_length = 400 directory = CASES_PATH / "message_changed" with patch( "pylint.testutils._primer.primer_compare_command.MAX_GITHUB_COMMENT_LENGTH", From 1389d74262a08a15a2c1342580327cb61fb2eb46 Mon Sep 17 00:00:00 2001 From: Pierre Sassoulas Date: Fri, 3 Apr 2026 10:00:21 +0200 Subject: [PATCH 3/8] [primer] Clarify location diff wording and robustify key iteration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - format_span now returns `line:col`-`endLine:endCol` spans, and message_diff says "Moved from {old} to {new}" — avoids ambiguity between span-range and movement. - message_diff iterates set(old) | set(new) so keys present only in new are also detected. --- pylint/testutils/_primer/comparator.py | 10 +++++----- tests/testutils/_primer/cases/line_moved/expected.txt | 2 +- .../_primer/cases/message_changed_line/expected.txt | 2 +- .../testutils/_primer/cases/mixed_changes/expected.txt | 2 +- .../testutils/_primer/cases/multi_package/expected.txt | 2 +- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/pylint/testutils/_primer/comparator.py b/pylint/testutils/_primer/comparator.py index 0b09a918ff..c5e7d2d037 100644 --- a/pylint/testutils/_primer/comparator.py +++ b/pylint/testutils/_primer/comparator.py @@ -75,8 +75,8 @@ def format_span(msg: JSONMessage) -> str: end_line = msg.get("endLine") end_col = msg.get("endColumn") if end_line is not None and end_col is not None: - return f"from `{start}` to `{end_line}:{end_col}`" - return f"at `{start}`" + return f"`{start}`-`{end_line}:{end_col}`" + return f"`{start}`" def message_diff(old: JSONMessage, new: JSONMessage) -> str: @@ -87,14 +87,14 @@ def message_diff(old: JSONMessage, new: JSONMessage) -> str: renders them with red/green highlighting. """ changed_keys: set[str] = set() - for key in old: - if old[key] != new[key]: # type: ignore[literal-required] + for key in set(old) | set(new): + if old.get(key) != new.get(key): changed_keys.add(key) parts: list[str] = [] # Location: combine line/column/endLine/endColumn into one sentence. if changed_keys & _LOCATION_KEYS: - parts.append(f"Was raised {format_span(old)}, now {format_span(new)}.") + parts.append(f"Moved from {format_span(old)} to {format_span(new)}.") # Other fields (typically ``message`` or ``type``). for key in sorted(changed_keys - _LOCATION_KEYS): diff --git a/tests/testutils/_primer/cases/line_moved/expected.txt b/tests/testutils/_primer/cases/line_moved/expected.txt index 03d388dea3..b52b2a9fc1 100644 --- a/tests/testutils/_primer/cases/line_moved/expected.txt +++ b/tests/testutils/_primer/cases/line_moved/expected.txt @@ -8,7 +8,7 @@ Changed messages:
1) [too-complex](https://github.com/pylint-dev/astroid/blob/123456789abcdef/astroid/node_classes.py#L103): -Was raised from `100:0` to `100:15`, now from `103:0` to `103:15`. +Moved from `100:0`-`100:15` to `103:0`-`103:15`.
*This comment was generated for commit v2.14.2* diff --git a/tests/testutils/_primer/cases/message_changed_line/expected.txt b/tests/testutils/_primer/cases/message_changed_line/expected.txt index aab58798d7..acb71c2498 100644 --- a/tests/testutils/_primer/cases/message_changed_line/expected.txt +++ b/tests/testutils/_primer/cases/message_changed_line/expected.txt @@ -8,7 +8,7 @@ Changed messages:
1) [too-complex](https://github.com/pylint-dev/astroid/blob/123456789abcdef/astroid/node_classes.py#L103): -Was raised from `100:0` to `100:15`, now from `103:0` to `103:15`. +Moved from `100:0`-`100:15` to `103:0`-`103:15`. ```diff - 'my_function' is too complex. The McCabe rating is 18 diff --git a/tests/testutils/_primer/cases/mixed_changes/expected.txt b/tests/testutils/_primer/cases/mixed_changes/expected.txt index b40a50c3a7..23511399cd 100644 --- a/tests/testutils/_primer/cases/mixed_changes/expected.txt +++ b/tests/testutils/_primer/cases/mixed_changes/expected.txt @@ -14,7 +14,7 @@ Changed messages: ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ``` 2) [too-complex](https://github.com/pylint-dev/astroid/blob/123456789abcdef/astroid/node_classes.py#L105): -Was raised from `100:0` to `100:15`, now from `105:0` to `105:15`. +Moved from `100:0`-`100:15` to `105:0`-`105:15`. confidence: `UNDEFINED` → `HIGH` diff --git a/tests/testutils/_primer/cases/multi_package/expected.txt b/tests/testutils/_primer/cases/multi_package/expected.txt index a4f6ea541f..67d8236e3b 100644 --- a/tests/testutils/_primer/cases/multi_package/expected.txt +++ b/tests/testutils/_primer/cases/multi_package/expected.txt @@ -8,7 +8,7 @@ Changed messages:
1) [too-complex](https://github.com/pylint-dev/astroid/blob/123456789abcdef/astroid/node_classes.py#L105): -Was raised from `100:0` to `100:15`, now from `105:0` to `105:15`. +Moved from `100:0`-`100:15` to `105:0`-`105:15`. ```diff - 'my_function' is too complex. The McCabe rating is 18 From c683e5d5c4803767018e92273cb8987f8676923f Mon Sep 17 00:00:00 2001 From: Pierre Sassoulas Date: Sat, 18 Apr 2026 14:19:59 +0200 Subject: [PATCH 4/8] [primer] Cover classification branches and truncation edge cases MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds fixtures and unit tests that bring coverage on comparator.py and primer_compare_command.py to 100% — all message-classification branches (astroid-error, useless-suppression fix, locally-disabled FP, genuine new message) and both truncation code paths (multi-package iteration break and no-space fallback) now have dedicated tests. --- doc/whatsnew/fragments/10914.internal | 7 ++ .../expected_truncated_break.txt | 7 +- .../new_messages_classified/expected.txt | 35 ++++++++++ .../expected_comparator.json | 1 + .../cases/new_messages_classified/main.json | 6 ++ .../cases/new_messages_classified/pr.json | 67 +++++++++++++++++++ tests/testutils/_primer/test_comparator.py | 40 ++++++++++- tests/testutils/_primer/test_primer.py | 2 +- 8 files changed, 159 insertions(+), 6 deletions(-) create mode 100644 doc/whatsnew/fragments/10914.internal create mode 100644 tests/testutils/_primer/cases/new_messages_classified/expected.txt create mode 100644 tests/testutils/_primer/cases/new_messages_classified/expected_comparator.json create mode 100644 tests/testutils/_primer/cases/new_messages_classified/main.json create mode 100644 tests/testutils/_primer/cases/new_messages_classified/pr.json diff --git a/doc/whatsnew/fragments/10914.internal b/doc/whatsnew/fragments/10914.internal new file mode 100644 index 0000000000..dc31fa513e --- /dev/null +++ b/doc/whatsnew/fragments/10914.internal @@ -0,0 +1,7 @@ +The primer now pairs residual messages by ``(symbol, path, obj)`` and reports altered +diagnostics as a single *changed* entry with a compact diff, rather than as a separate +removal + addition. New messages are classified into genuine new messages, fixed false +positives (``useless-suppression``), new false positives (``locally-disabled``), and +``astroid-error`` fatal errors. + +Refs #10914 diff --git a/tests/testutils/_primer/cases/multi_package/expected_truncated_break.txt b/tests/testutils/_primer/cases/multi_package/expected_truncated_break.txt index 331f74ff7f..a285996bf7 100644 --- a/tests/testutils/_primer/cases/multi_package/expected_truncated_break.txt +++ b/tests/testutils/_primer/cases/multi_package/expected_truncated_break.txt @@ -3,14 +3,13 @@ **Effect on [astroid](https://github.com/pylint-dev/astroid):** -The following messages are now emitted: +Changed messages:
-1) too-complex: -*'my_function' is too complex. The McCabe rating is... +1)...
-*This comment was truncated because GitHub allows only 500 characters in a comment.* +*This comment was truncated because GitHub allows only 400 characters in a comment.* *This comment was generated for commit v2.14.2* diff --git a/tests/testutils/_primer/cases/new_messages_classified/expected.txt b/tests/testutils/_primer/cases/new_messages_classified/expected.txt new file mode 100644 index 0000000000..cfee18bc0e --- /dev/null +++ b/tests/testutils/_primer/cases/new_messages_classified/expected.txt @@ -0,0 +1,35 @@ +🤖 **Effect of this PR on checked open source code:** 🤖 + + +**Effect on [astropy](https://github.com/astropy/astropy):** + +1 "astroid error(s)" were found. Please open the GitHub Actions log to see what failed or crashed. + +🎉 Fixed false positives: + +
+ +1) useless-suppression: +*Useless suppression of 'looping-through-iterator'* +https://github.com/astropy/astropy/blob/1fb40bc1f22f176254ef583065aa155f53f3b414/astropy/coordinates/angles/angle_parsetab.py#L14 +
+ +😞 New false positives: + +
+ +1) locally-disabled: +*Locally disabling looping-through-iterator (W4801)* +https://github.com/astropy/astropy/blob/1fb40bc1f22f176254ef583065aa155f53f3b414/astropy/coordinates/angles/angle_parsetab.py#L14 +
+ +New messages: + +
+ +1) unused-variable: +*Unused variable 'foo'* +https://github.com/astropy/astropy/blob/1fb40bc1f22f176254ef583065aa155f53f3b414/astropy/coordinates/angles/angle_parsetab.py#L42 +
+ +*This comment was generated for commit v2.14.2* diff --git a/tests/testutils/_primer/cases/new_messages_classified/expected_comparator.json b/tests/testutils/_primer/cases/new_messages_classified/expected_comparator.json new file mode 100644 index 0000000000..bb57521031 --- /dev/null +++ b/tests/testutils/_primer/cases/new_messages_classified/expected_comparator.json @@ -0,0 +1 @@ +[{ "package": "astropy", "missing": 0, "new": 4, "changed": 0 }] diff --git a/tests/testutils/_primer/cases/new_messages_classified/main.json b/tests/testutils/_primer/cases/new_messages_classified/main.json new file mode 100644 index 0000000000..337476e532 --- /dev/null +++ b/tests/testutils/_primer/cases/new_messages_classified/main.json @@ -0,0 +1,6 @@ +{ + "astropy": { + "commit": "1fb40bc1f22f176254ef583065aa155f53f3b414", + "messages": [] + } +} diff --git a/tests/testutils/_primer/cases/new_messages_classified/pr.json b/tests/testutils/_primer/cases/new_messages_classified/pr.json new file mode 100644 index 0000000000..238805c199 --- /dev/null +++ b/tests/testutils/_primer/cases/new_messages_classified/pr.json @@ -0,0 +1,67 @@ +{ + "astropy": { + "commit": "1fb40bc1f22f176254ef583065aa155f53f3b414", + "messages": [ + { + "type": "error", + "module": "astropy.coordinates.angles.angle_parsetab", + "obj": "", + "line": 1, + "column": 0, + "endLine": null, + "endColumn": null, + "path": "tests/.pylint_primer_tests/astropy/astropy/astropy/coordinates/angles/angle_parsetab.py", + "symbol": "astroid-error", + "message": "Fatal error while checking 'astropy/module.py'. Please open an issue.", + "messageId": "F0002", + "confidence": "UNDEFINED", + "absolutePath": "tests/.pylint_primer_tests/astropy/astropy/astropy/coordinates/angles/angle_parsetab.py" + }, + { + "type": "info", + "module": "astropy.coordinates.angles.angle_parsetab", + "obj": "", + "line": 14, + "column": 0, + "endLine": null, + "endColumn": null, + "path": "tests/.pylint_primer_tests/astropy/astropy/astropy/coordinates/angles/angle_parsetab.py", + "symbol": "locally-disabled", + "message": "Locally disabling looping-through-iterator (W4801)", + "messageId": "I0011", + "confidence": "UNDEFINED", + "absolutePath": "tests/.pylint_primer_tests/astropy/astropy/astropy/coordinates/angles/angle_parsetab.py" + }, + { + "type": "warning", + "module": "astropy.coordinates.angles.angle_parsetab", + "obj": "", + "line": 14, + "column": 0, + "endLine": null, + "endColumn": null, + "path": "tests/.pylint_primer_tests/astropy/astropy/astropy/coordinates/angles/angle_parsetab.py", + "symbol": "useless-suppression", + "message": "Useless suppression of 'looping-through-iterator'", + "messageId": "I0021", + "confidence": "UNDEFINED", + "absolutePath": "tests/.pylint_primer_tests/astropy/astropy/astropy/coordinates/angles/angle_parsetab.py" + }, + { + "type": "warning", + "module": "astropy.coordinates.angles.angle_parsetab", + "obj": "my_function", + "line": 42, + "column": 4, + "endLine": 42, + "endColumn": 16, + "path": "tests/.pylint_primer_tests/astropy/astropy/astropy/coordinates/angles/angle_parsetab.py", + "symbol": "unused-variable", + "message": "Unused variable 'foo'", + "messageId": "W0612", + "confidence": "UNDEFINED", + "absolutePath": "tests/.pylint_primer_tests/astropy/astropy/astropy/coordinates/angles/angle_parsetab.py" + } + ] + } +} diff --git a/tests/testutils/_primer/test_comparator.py b/tests/testutils/_primer/test_comparator.py index 94e8d035a0..ace10ba19c 100644 --- a/tests/testutils/_primer/test_comparator.py +++ b/tests/testutils/_primer/test_comparator.py @@ -4,14 +4,40 @@ import json from pathlib import Path +from typing import cast import pytest -from pylint.testutils._primer.comparator import Comparator +from pylint.reporters.json_reporter import JSONMessage +from pylint.testutils._primer.comparator import ( + Comparator, + _caret_hint, + format_span, +) CASES_PATH = Path(__file__).parent / "cases" +def _msg(**overrides: object) -> JSONMessage: + base: dict[str, object] = { + "type": "warning", + "module": "m", + "obj": "", + "line": 1, + "column": 0, + "endLine": 1, + "endColumn": 5, + "path": "m.py", + "symbol": "s", + "message": "msg", + "messageId": "W0000", + "confidence": "UNDEFINED", + "absolutePath": "m.py", + } + base.update(overrides) + return cast(JSONMessage, base) + + @pytest.mark.parametrize( "directory", [pytest.param(p, id=p.name) for p in CASES_PATH.iterdir() if p.is_dir()], @@ -29,6 +55,18 @@ def test_comparator(directory: Path) -> None: assert len(changed) == exp["changed"] +def test_format_span_without_end_position() -> None: + """Messages missing endLine/endColumn fall back to a start-only span.""" + assert ( + format_span(_msg(line=42, column=7, endLine=None, endColumn=None)) == "`42:7`" + ) + + +def test_caret_hint_returns_empty_when_mostly_changed() -> None: + """When more than 60% of the text differs, carets are noise and are skipped.""" + assert _caret_hint("hello world", "goodbye universe") == "" + + def test_comparator_batched() -> None: fixture = Path(__file__).parent / "batched_cases" comparator = Comparator.from_json( diff --git a/tests/testutils/_primer/test_primer.py b/tests/testutils/_primer/test_primer.py index d443cde693..69f675f418 100644 --- a/tests/testutils/_primer/test_primer.py +++ b/tests/testutils/_primer/test_primer.py @@ -90,7 +90,7 @@ def test_truncated_compare(self) -> None: def test_truncated_compare_stops_iterating_packages(self) -> None: """Once the comment exceeds MAX, further packages should be skipped.""" - max_comment_length = 500 + max_comment_length = 400 directory = CASES_PATH / "multi_package" with patch( "pylint.testutils._primer.primer_compare_command.MAX_GITHUB_COMMENT_LENGTH", From 47f2682851ff0761147b25fbb88e1e575b036e05 Mon Sep 17 00:00:00 2001 From: Pierre Sassoulas Date: Sat, 18 Apr 2026 20:46:24 +0200 Subject: [PATCH 5/8] [primer] Thread ChangedMessage through message_diff and split comment helpers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pass ``ChangedMessage`` to ``message_diff`` directly so callers don't have to unpack ``(old, new)`` at every site. Rename ``_match_residuals`` to ``_pair_by_position`` — it advertises what the helper does rather than what it consumes. Split the collapsed ``_create_comment_for_package`` back into focused ``_format_changed_messages`` / ``_format_new_messages`` / ``_format_missing_messages`` helpers, with a small ``_source_link_for`` factory for the per-package closure. Drops the ``_ClassifiedMessages`` side-car class; classification lives inline in the new-messages formatter it belongs to. Also swap "diagnostic" for "message"/"warning" in docstrings and comments to match pylint's own terminology. --- doc/whatsnew/fragments/10914.internal | 2 +- pylint/testutils/_primer/comparator.py | 21 +- .../_primer/primer_compare_command.py | 188 +++++++++--------- 3 files changed, 99 insertions(+), 112 deletions(-) diff --git a/doc/whatsnew/fragments/10914.internal b/doc/whatsnew/fragments/10914.internal index dc31fa513e..009351e592 100644 --- a/doc/whatsnew/fragments/10914.internal +++ b/doc/whatsnew/fragments/10914.internal @@ -1,5 +1,5 @@ The primer now pairs residual messages by ``(symbol, path, obj)`` and reports altered -diagnostics as a single *changed* entry with a compact diff, rather than as a separate +messages as a single *changed* entry with a compact diff, rather than as a separate removal + addition. New messages are classified into genuine new messages, fixed false positives (``useless-suppression``), new false positives (``locally-disabled``), and ``astroid-error`` fatal errors. diff --git a/pylint/testutils/_primer/comparator.py b/pylint/testutils/_primer/comparator.py index c5e7d2d037..d361ff45a9 100644 --- a/pylint/testutils/_primer/comparator.py +++ b/pylint/testutils/_primer/comparator.py @@ -31,15 +31,15 @@ class PackageDiff(NamedTuple): package: str missing: PackageData # emitted on main but not on the PR new: PackageData # emitted on the PR but not on main - changed: list[ChangedMessage] # same diagnostic, altered line / text + changed: list[ChangedMessage] # same message, altered line / text -def _match_residuals( +def _pair_by_position( old_residuals: list[JSONMessage], new_residuals: list[JSONMessage] ) -> tuple[list[ChangedMessage], list[JSONMessage], list[JSONMessage]]: """Pair residual messages by ``(symbol, path, obj)``. - Two messages sharing that key are the "same diagnostic" — if they differ + Two messages sharing that key are the "same warning" — if they differ only in line numbers or message text, they should be reported as *changed* rather than as a separate removal + addition. Leftovers on each side are genuinely missing or genuinely new. @@ -79,13 +79,14 @@ def format_span(msg: JSONMessage) -> str: return f"`{start}`" -def message_diff(old: JSONMessage, new: JSONMessage) -> str: +def message_diff(change: ChangedMessage) -> str: """Return a compact summary of changed fields between two messages. Location changes are merged into a single human-readable span. String fields (like ``message``) get a ``diff`` code block so GitHub renders them with red/green highlighting. """ + old, new = change.old, change.new changed_keys: set[str] = set() for key in set(old) | set(new): if old.get(key) != new.get(key): @@ -138,13 +139,7 @@ def _caret_hint(old: str, new: str) -> str: class Comparator: - """Cross-reference two primer JSON outputs and iterate over differences. - - Yields ``PackageDiff`` entries carrying missing, new and changed messages - for each package that has at least one difference. *changed* contains - pairs of ``(old, new)`` messages that are the same diagnostic but with - altered details (line, message text, etc.). - """ + """Cross-reference two primer JSON outputs and iterate over differences.""" def __init__(self, main_data: PackageMessages, pr_data: PackageMessages) -> None: self._main_data = main_data @@ -188,8 +183,8 @@ def __iter__(self) -> Iterator[PackageDiff]: residual_old.append(message) # Second pass: pair residuals by position to detect *changed* - # messages (same diagnostic, different line or text). - paired, final_missing, final_new = _match_residuals( + # messages (same warning, different line or text). + paired, final_missing, final_new = _pair_by_position( residual_old, pr_messages ) diff --git a/pylint/testutils/_primer/primer_compare_command.py b/pylint/testutils/_primer/primer_compare_command.py index f5df3fc3f0..af6a6794a7 100644 --- a/pylint/testutils/_primer/primer_compare_command.py +++ b/pylint/testutils/_primer/primer_compare_command.py @@ -11,7 +11,6 @@ from pylint.testutils._primer.comparator import ( ChangedMessage, Comparator, - PackageDiff, message_diff, ) from pylint.testutils._primer.primer_command import PrimerCommand @@ -33,54 +32,9 @@ def _format_messages( return body -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[JSONMessage], - missing_messages: list[JSONMessage], - changed_messages: list[ChangedMessage], - source_link: Callable[[JSONMessage], str], - ) -> None: - astroid_errors = 0 - new_fp: list[JSONMessage] = [] - fixed_fp: list[JSONMessage] = [] - other_new: list[JSONMessage] = [] - 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) - - self.astroid_errors = astroid_errors - self.fixed_false_positives = _format_messages(fixed_fp, source_link) - self.new_false_positives = _format_messages(new_fp, source_link) - self.new_messages = _format_messages(other_new, source_link) - self.missing_messages = _format_messages(missing_messages, source_link) - - 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 +def _details_section(title: str, body: str) -> str: + # Blank line after
required for GitHub markdown rendering. + return f"{title}\n\n
\n\n{body}
\n\n" class CompareCommand(PrimerCommand): @@ -97,7 +51,18 @@ def _create_comment(self, comparator: Comparator) -> str: for diff in comparator: if len(comment) >= MAX_GITHUB_COMMENT_LENGTH: break - comment += self._create_comment_for_package(diff) + package = diff.package + url = self.packages[package].url + assert not url.endswith( + ".git" + ), "You don't need the .git at the end of the github url." + source_link = self._source_link_for(package, diff.new["commit"]) + comment += f"\n**Effect on [{package}]({url}):**\n\n" + comment += self._format_changed_messages(diff.changed, source_link) + comment += self._format_new_messages(diff.new["messages"], source_link) + comment += self._format_missing_messages( + diff.missing["messages"], source_link + ) comment = ( f"🤖 **Effect of this PR on checked open source code:** 🤖\n\n{comment}" if comment @@ -108,66 +73,93 @@ def _create_comment(self, comparator: Comparator) -> str: ) return self._truncate_comment(comment) - def _create_comment_for_package(self, diff: PackageDiff) -> str: - package = diff.package - url = self.packages[package].url + def _source_link_for( + self, package: str, commit: str + ) -> Callable[[JSONMessage], str]: clone_dir = self.packages[package].clone_directory - commit = diff.new["commit"] - - assert not url.endswith( - ".git" - ), "You don't need the .git at the end of the github url." + url = self.packages[package].url - def _source_link(msg: JSONMessage) -> str: + def _link(msg: JSONMessage) -> str: filepath = str(PurePosixPath(msg["path"]).relative_to(clone_dir)) return f"{url}/blob/{commit}/{filepath}#L{msg['line']}" - def _details_section(title: str, body: str) -> str: - # Blank line after
required for GitHub markdown rendering. - return f"{title}\n\n
\n\n{body}
\n\n" + return _link - classified = _ClassifiedMessages( - diff.new["messages"], diff.missing["messages"], diff.changed, _source_link - ) - comment = f"\n**Effect on [{package}]({url}):**\n\n" - - if diff.changed: - print("Changed:") - for _, new in diff.changed: - print(new) - if diff.new["messages"]: - print("Now emitted:") - for msg in diff.new["messages"]: - print(msg) - if diff.missing["messages"]: - print("No longer emitted:") - for msg in diff.missing["messages"]: - print(msg) - - if classified.changed_messages: - comment += _details_section( - "Changed messages:", classified.changed_messages + def _format_changed_messages( + self, + changed: list[ChangedMessage], + source_link: Callable[[JSONMessage], str], + ) -> str: + if not changed: + return "" + print("Changed:") + body = "" + for count, change in enumerate(changed, 1): + print(change.new) + body += ( + f"{count}) [{change.new['symbol']}]({source_link(change.new)}):\n" + f"{message_diff(change)}\n" ) - if classified.astroid_errors: - comment += ( - f'{classified.astroid_errors} "astroid error(s)" were found. ' + return _details_section("Changed messages:", body) + + def _format_new_messages( + self, + messages: list[JSONMessage], + source_link: Callable[[JSONMessage], str], + ) -> str: + if not messages: + return "" + print("Now emitted:") + astroid_errors = 0 + fixed_fp: list[JSONMessage] = [] + new_fp: list[JSONMessage] = [] + other_new: list[JSONMessage] = [] + for message in messages: + print(message) + 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) + + out = "" + if astroid_errors: + out += ( + f'{astroid_errors} "astroid error(s)" were found. ' "Please open the GitHub Actions log to see what failed or crashed.\n\n" ) - if classified.fixed_false_positives: - comment += _details_section( - "🎉 Fixed false positives:", classified.fixed_false_positives + if fixed_fp: + out += _details_section( + "🎉 Fixed false positives:", _format_messages(fixed_fp, source_link) ) - if classified.new_false_positives: - comment += _details_section( - "😞 New false positives:", classified.new_false_positives + if new_fp: + out += _details_section( + "😞 New false positives:", _format_messages(new_fp, source_link) ) - if classified.new_messages: - comment += _details_section("New messages:", classified.new_messages) - if classified.missing_messages: - comment += _details_section( - "Removed messages:", classified.missing_messages + if other_new: + out += _details_section( + "New messages:", _format_messages(other_new, source_link) ) - return comment + return out + + def _format_missing_messages( + self, + messages: list[JSONMessage], + source_link: Callable[[JSONMessage], str], + ) -> str: + if not messages: + return "" + print("No longer emitted:") + for message in messages: + print(message) + return _details_section( + "Removed messages:", _format_messages(messages, source_link) + ) def _truncate_comment(self, comment: str) -> str: """GitHub allows only a set number of characters in a comment.""" From 6705d74f786009330c0fb32892990606ea833e9b Mon Sep 17 00:00:00 2001 From: Pierre Sassoulas Date: Sun, 19 Apr 2026 09:13:37 +0200 Subject: [PATCH 6/8] [primer] Simplify residual pairing to single-sided bucketing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bucket only the *new* side and walk ``old_residuals`` once, popping from each matching bucket. That removes the dual ``old_by_key`` dictionary and the ``paired_old_ids`` / ``paired_new_ids`` shadow bookkeeping — we can build ``final_missing`` inline in the old-side walk, and ``final_new`` still reads from the untouched input list. --- pylint/testutils/_primer/comparator.py | 34 +++++++++++--------------- 1 file changed, 14 insertions(+), 20 deletions(-) diff --git a/pylint/testutils/_primer/comparator.py b/pylint/testutils/_primer/comparator.py index d361ff45a9..2112253650 100644 --- a/pylint/testutils/_primer/comparator.py +++ b/pylint/testutils/_primer/comparator.py @@ -37,35 +37,29 @@ class PackageDiff(NamedTuple): def _pair_by_position( old_residuals: list[JSONMessage], new_residuals: list[JSONMessage] ) -> tuple[list[ChangedMessage], list[JSONMessage], list[JSONMessage]]: - """Pair residual messages by ``(symbol, path, obj)``. + """Pair residual messages by ``(symbol, path, obj)`` in input order. - Two messages sharing that key are the "same warning" — if they differ - only in line numbers or message text, they should be reported as *changed* + Two messages sharing that key are the "same warning" — if they differ only + in line numbers or message text, they should be reported as *changed* rather than as a separate removal + addition. Leftovers on each side are genuinely missing or genuinely new. - - Returns ``(paired, unmatched_old, unmatched_new)``. """ - old_by_key: dict[tuple[str, str, str], list[JSONMessage]] = defaultdict(list) new_by_key: dict[tuple[str, str, str], list[JSONMessage]] = defaultdict(list) - for m in old_residuals: - old_by_key[m["symbol"], m["path"], m["obj"]].append(m) for m in new_residuals: new_by_key[m["symbol"], m["path"], m["obj"]].append(m) paired: list[ChangedMessage] = [] - paired_old_ids: set[int] = set() - paired_new_ids: set[int] = set() - for key in old_by_key: - if key not in new_by_key: - continue - for old, new in zip(old_by_key[key], new_by_key[key]): + final_missing: list[JSONMessage] = [] + matched_new: set[int] = set() + for old in old_residuals: + bucket = new_by_key.get((old["symbol"], old["path"], old["obj"])) + if bucket: + new = bucket.pop(0) paired.append(ChangedMessage(old=old, new=new)) - paired_old_ids.add(id(old)) - paired_new_ids.add(id(new)) - - final_missing = [m for m in old_residuals if id(m) not in paired_old_ids] - final_new = [m for m in new_residuals if id(m) not in paired_new_ids] + matched_new.add(id(new)) + else: + final_missing.append(old) + final_new = [m for m in new_residuals if id(m) not in matched_new] return paired, final_missing, final_new @@ -88,7 +82,7 @@ def message_diff(change: ChangedMessage) -> str: """ old, new = change.old, change.new changed_keys: set[str] = set() - for key in set(old) | set(new): + for key in old.keys() | new.keys(): if old.get(key) != new.get(key): changed_keys.add(key) From 6e7d3c9231cfba6afb89aec833bc07e4614eacbe Mon Sep 17 00:00:00 2001 From: Pierre Sassoulas Date: Sun, 26 Apr 2026 14:08:33 +0200 Subject: [PATCH 7/8] [primer] Pair symbol-renamed messages at the same location MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a second pairing pass keyed on exact source location, gated on ``SequenceMatcher.ratio() >= 0.5`` between the old and new symbols. This catches renames like ``used-before-assignment`` → ``possibly-used-before-assignment`` and reports them as one *changed* message instead of a separate removal + addition. The similarity gate avoids pairing unrelated messages that happen to coincide on the same line (e.g. ``useless-suppression`` removed and ``locally-disabled`` added at the same position). --- doc/whatsnew/fragments/10914.internal | 12 +-- pylint/testutils/_primer/comparator.py | 78 +++++++++++++++---- .../_primer/cases/symbol_renamed/expected.txt | 26 +++++++ .../symbol_renamed/expected_comparator.json | 1 + .../_primer/cases/symbol_renamed/main.json | 22 ++++++ .../_primer/cases/symbol_renamed/pr.json | 22 ++++++ tests/testutils/_primer/test_comparator.py | 16 ++++ 7 files changed, 159 insertions(+), 18 deletions(-) create mode 100644 tests/testutils/_primer/cases/symbol_renamed/expected.txt create mode 100644 tests/testutils/_primer/cases/symbol_renamed/expected_comparator.json create mode 100644 tests/testutils/_primer/cases/symbol_renamed/main.json create mode 100644 tests/testutils/_primer/cases/symbol_renamed/pr.json diff --git a/doc/whatsnew/fragments/10914.internal b/doc/whatsnew/fragments/10914.internal index 009351e592..35ac3db975 100644 --- a/doc/whatsnew/fragments/10914.internal +++ b/doc/whatsnew/fragments/10914.internal @@ -1,7 +1,9 @@ -The primer now pairs residual messages by ``(symbol, path, obj)`` and reports altered -messages as a single *changed* entry with a compact diff, rather than as a separate -removal + addition. New messages are classified into genuine new messages, fixed false -positives (``useless-suppression``), new false positives (``locally-disabled``), and -``astroid-error`` fatal errors. +The primer now pairs residual messages — first by ``(symbol, path, obj)`` and then by +exact source location — and reports altered messages as a single *changed* entry with +a compact diff, rather than as a separate removal + addition. The location-based pass +also catches symbol renames at the same code position (e.g. ``used-before-assignment`` +→ ``possibly-used-before-assignment``). New messages are classified into genuine new +messages, fixed false positives (``useless-suppression``), new false positives +(``locally-disabled``), and ``astroid-error`` fatal errors. Refs #10914 diff --git a/pylint/testutils/_primer/comparator.py b/pylint/testutils/_primer/comparator.py index 2112253650..6feb4f744f 100644 --- a/pylint/testutils/_primer/comparator.py +++ b/pylint/testutils/_primer/comparator.py @@ -6,7 +6,7 @@ import json from collections import defaultdict -from collections.abc import Iterator +from collections.abc import Callable, Hashable, Iterator from difflib import SequenceMatcher from pathlib import Path from typing import NamedTuple @@ -34,26 +34,35 @@ class PackageDiff(NamedTuple): changed: list[ChangedMessage] # same message, altered line / text -def _pair_by_position( - old_residuals: list[JSONMessage], new_residuals: list[JSONMessage] +SYMBOL_RENAME_SIMILARITY_THRESHOLD = 0.5 + + +def _pair_by_key( + old_residuals: list[JSONMessage], + new_residuals: list[JSONMessage], + key: Callable[[JSONMessage], tuple[Hashable, ...]], + accept: Callable[[JSONMessage, JSONMessage], bool] = lambda _o, _n: True, ) -> tuple[list[ChangedMessage], list[JSONMessage], list[JSONMessage]]: - """Pair residual messages by ``(symbol, path, obj)`` in input order. + """Pair messages whose ``key`` matches, in input order. - Two messages sharing that key are the "same warning" — if they differ only + Two messages sharing the key are the "same warning" — if they differ only in line numbers or message text, they should be reported as *changed* rather than as a separate removal + addition. Leftovers on each side are - genuinely missing or genuinely new. + passed through to the next pass (or reported as missing/new). + + ``accept`` is an optional predicate that can veto a candidate pair (used to + avoid pairing two unrelated messages that happen to share the key). """ - new_by_key: dict[tuple[str, str, str], list[JSONMessage]] = defaultdict(list) + new_by_key: dict[tuple[Hashable, ...], list[JSONMessage]] = defaultdict(list) for m in new_residuals: - new_by_key[m["symbol"], m["path"], m["obj"]].append(m) + new_by_key[key(m)].append(m) paired: list[ChangedMessage] = [] final_missing: list[JSONMessage] = [] matched_new: set[int] = set() for old in old_residuals: - bucket = new_by_key.get((old["symbol"], old["path"], old["obj"])) - if bucket: + bucket = new_by_key.get(key(old)) + if bucket and accept(old, bucket[0]): new = bucket.pop(0) paired.append(ChangedMessage(old=old, new=new)) matched_new.add(id(new)) @@ -63,6 +72,48 @@ def _pair_by_position( return paired, final_missing, final_new +def _is_symbol_rename(old: JSONMessage, new: JSONMessage) -> bool: + """True when the two symbols are textually similar enough to be a rename. + + Used to avoid pairing unrelated messages that happen to share a source + position (e.g. ``useless-suppression`` removed and ``locally-disabled`` + added on the same line — those are conceptually opposite, not a rename). + """ + ratio = SequenceMatcher(None, old["symbol"], new["symbol"]).ratio() + return ratio >= SYMBOL_RENAME_SIMILARITY_THRESHOLD + + +def _pair_residuals( + old_residuals: list[JSONMessage], new_residuals: list[JSONMessage] +) -> tuple[list[ChangedMessage], list[JSONMessage], list[JSONMessage]]: + """Pair residual messages with two passes of decreasing strictness. + + First by ``(symbol, path, obj)`` — catches line-only or message-only changes + for the same warning. Then by exact source location, gated on symbol + similarity — catches symbol renames such as ``used-before-assignment`` → + ``possibly-used-before-assignment``, where the same code position now + emits a renamed message. + """ + paired_by_symbol, missing, new = _pair_by_key( + old_residuals, + new_residuals, + key=lambda m: (m["symbol"], m["path"], m["obj"]), + ) + paired_by_location, missing, new = _pair_by_key( + missing, + new, + key=lambda m: ( + m["path"], + m["line"], + m["column"], + m.get("endLine"), + m.get("endColumn"), + ), + accept=_is_symbol_rename, + ) + return paired_by_symbol + paired_by_location, missing, new + + def format_span(msg: JSONMessage) -> str: """Format a message's location as ``line:col to endLine:endCol``.""" start = f"{msg['line']}:{msg['column']}" @@ -176,9 +227,10 @@ def __iter__(self) -> Iterator[PackageDiff]: except ValueError: residual_old.append(message) - # Second pass: pair residuals by position to detect *changed* - # messages (same warning, different line or text). - paired, final_missing, final_new = _pair_by_position( + # Second pass: pair residuals by symbol then by location to + # detect *changed* messages (same warning, different line/text, + # or a symbol rename at the same source position). + paired, final_missing, final_new = _pair_residuals( residual_old, pr_messages ) diff --git a/tests/testutils/_primer/cases/symbol_renamed/expected.txt b/tests/testutils/_primer/cases/symbol_renamed/expected.txt new file mode 100644 index 0000000000..5c723cc900 --- /dev/null +++ b/tests/testutils/_primer/cases/symbol_renamed/expected.txt @@ -0,0 +1,26 @@ +🤖 **Effect of this PR on checked open source code:** 🤖 + + +**Effect on [astroid](https://github.com/pylint-dev/astroid):** + +Changed messages: + +
+ +1) [possibly-used-before-assignment](https://github.com/pylint-dev/astroid/blob/123456789abcdef/astroid/wcs.py#L3072): +confidence: `HIGH` → `INFERENCE` + +```diff +- Using variable 'orig_key' before assignment ++ Possibly using variable 'orig_key' before assignment + ^^^^^^^^^^ +``` + +messageId: `E0601` → `E0606` + +symbol: `used-before-assignment` → `possibly-used-before-assignment` + +type: `error` → `warning` +
+ +*This comment was generated for commit v2.14.2* diff --git a/tests/testutils/_primer/cases/symbol_renamed/expected_comparator.json b/tests/testutils/_primer/cases/symbol_renamed/expected_comparator.json new file mode 100644 index 0000000000..47824cb2e1 --- /dev/null +++ b/tests/testutils/_primer/cases/symbol_renamed/expected_comparator.json @@ -0,0 +1 @@ +[{ "package": "astroid", "missing": 0, "new": 0, "changed": 1 }] diff --git a/tests/testutils/_primer/cases/symbol_renamed/main.json b/tests/testutils/_primer/cases/symbol_renamed/main.json new file mode 100644 index 0000000000..e655ae656d --- /dev/null +++ b/tests/testutils/_primer/cases/symbol_renamed/main.json @@ -0,0 +1,22 @@ +{ + "astroid": { + "commit": "1234567890abcdef", + "messages": [ + { + "type": "error", + "module": "astroid.wcs", + "obj": "WCS.fix", + "line": 3072, + "column": 12, + "endLine": 3072, + "endColumn": 20, + "path": "tests/.pylint_primer_tests/pylint-dev/astroid/astroid/wcs.py", + "symbol": "used-before-assignment", + "message": "Using variable 'orig_key' before assignment", + "messageId": "E0601", + "confidence": "HIGH", + "absolutePath": "tests/.pylint_primer_tests/pylint-dev/astroid/astroid/wcs.py" + } + ] + } +} diff --git a/tests/testutils/_primer/cases/symbol_renamed/pr.json b/tests/testutils/_primer/cases/symbol_renamed/pr.json new file mode 100644 index 0000000000..3b90e6c589 --- /dev/null +++ b/tests/testutils/_primer/cases/symbol_renamed/pr.json @@ -0,0 +1,22 @@ +{ + "astroid": { + "commit": "123456789abcdef", + "messages": [ + { + "type": "warning", + "module": "astroid.wcs", + "obj": "WCS.fix", + "line": 3072, + "column": 12, + "endLine": 3072, + "endColumn": 20, + "path": "tests/.pylint_primer_tests/pylint-dev/astroid/astroid/wcs.py", + "symbol": "possibly-used-before-assignment", + "message": "Possibly using variable 'orig_key' before assignment", + "messageId": "E0606", + "confidence": "INFERENCE", + "absolutePath": "tests/.pylint_primer_tests/pylint-dev/astroid/astroid/wcs.py" + } + ] + } +} diff --git a/tests/testutils/_primer/test_comparator.py b/tests/testutils/_primer/test_comparator.py index ace10ba19c..c672a0fcea 100644 --- a/tests/testutils/_primer/test_comparator.py +++ b/tests/testutils/_primer/test_comparator.py @@ -12,6 +12,7 @@ from pylint.testutils._primer.comparator import ( Comparator, _caret_hint, + _is_symbol_rename, format_span, ) @@ -67,6 +68,21 @@ def test_caret_hint_returns_empty_when_mostly_changed() -> None: assert _caret_hint("hello world", "goodbye universe") == "" +@pytest.mark.parametrize( + ("old_symbol", "new_symbol", "expected"), + [ + ("used-before-assignment", "possibly-used-before-assignment", True), + ("abstract-method", "no-abstract-method", True), + ("useless-suppression", "locally-disabled", False), + ("too-complex", "too-many-locals", False), + ], +) +def test_is_symbol_rename(old_symbol: str, new_symbol: str, expected: bool) -> None: + old = _msg(symbol=old_symbol) + new = _msg(symbol=new_symbol) + assert _is_symbol_rename(old, new) is expected + + def test_comparator_batched() -> None: fixture = Path(__file__).parent / "batched_cases" comparator = Comparator.from_json( From 47eda507b35b76bb109ba8c6f7c00541bbe1d940 Mon Sep 17 00:00:00 2001 From: Pierre Sassoulas Date: Sun, 26 Apr 2026 14:13:01 +0200 Subject: [PATCH 8/8] [primer] Drop the ``locally-disabled`` new false positive bucket MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A ``locally-disabled`` message is emitted from a ``# pylint: disable=`` comment in the source. Since the primer pins each repository to a fixed commit, the disable comments are identical between the main and PR runs, so the original "new locally-disabled = newly-introduced false positive" assumption never fires in practice — and a maintainer upgrading pylint would clean those up anyway. Drop the dedicated bucket and let any new ``locally-disabled`` fall through to the generic *New messages* section. --- doc/whatsnew/fragments/10914.internal | 5 ++--- pylint/testutils/_primer/primer_compare_command.py | 9 --------- .../testutils/_primer/cases/multi_package/expected.txt | 2 +- tests/testutils/_primer/cases/new_message/expected.txt | 2 +- .../_primer/cases/new_messages_classified/expected.txt | 10 ++-------- .../_primer/cases/useless_suppression/expected.txt | 2 +- 6 files changed, 7 insertions(+), 23 deletions(-) diff --git a/doc/whatsnew/fragments/10914.internal b/doc/whatsnew/fragments/10914.internal index 35ac3db975..05947c343b 100644 --- a/doc/whatsnew/fragments/10914.internal +++ b/doc/whatsnew/fragments/10914.internal @@ -2,8 +2,7 @@ The primer now pairs residual messages — first by ``(symbol, path, obj)`` and exact source location — and reports altered messages as a single *changed* entry with a compact diff, rather than as a separate removal + addition. The location-based pass also catches symbol renames at the same code position (e.g. ``used-before-assignment`` -→ ``possibly-used-before-assignment``). New messages are classified into genuine new -messages, fixed false positives (``useless-suppression``), new false positives -(``locally-disabled``), and ``astroid-error`` fatal errors. +→ ``possibly-used-before-assignment``). New messages are classified into fixed false +positives (``useless-suppression``), ``astroid-error`` fatal errors, and the rest. Refs #10914 diff --git a/pylint/testutils/_primer/primer_compare_command.py b/pylint/testutils/_primer/primer_compare_command.py index af6a6794a7..222ad09a33 100644 --- a/pylint/testutils/_primer/primer_compare_command.py +++ b/pylint/testutils/_primer/primer_compare_command.py @@ -112,7 +112,6 @@ def _format_new_messages( print("Now emitted:") astroid_errors = 0 fixed_fp: list[JSONMessage] = [] - new_fp: list[JSONMessage] = [] other_new: list[JSONMessage] = [] for message in messages: print(message) @@ -120,10 +119,6 @@ def _format_new_messages( 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) @@ -137,10 +132,6 @@ def _format_new_messages( out += _details_section( "🎉 Fixed false positives:", _format_messages(fixed_fp, source_link) ) - if new_fp: - out += _details_section( - "😞 New false positives:", _format_messages(new_fp, source_link) - ) if other_new: out += _details_section( "New messages:", _format_messages(other_new, source_link) diff --git a/tests/testutils/_primer/cases/multi_package/expected.txt b/tests/testutils/_primer/cases/multi_package/expected.txt index 67d8236e3b..5da2087bc3 100644 --- a/tests/testutils/_primer/cases/multi_package/expected.txt +++ b/tests/testutils/_primer/cases/multi_package/expected.txt @@ -20,7 +20,7 @@ Moved from `100:0`-`100:15` to `105:0`-`105:15`. **Effect on [astropy](https://github.com/astropy/astropy):** -😞 New false positives: +New messages:
diff --git a/tests/testutils/_primer/cases/new_message/expected.txt b/tests/testutils/_primer/cases/new_message/expected.txt index e813dd0a86..040f70014a 100644 --- a/tests/testutils/_primer/cases/new_message/expected.txt +++ b/tests/testutils/_primer/cases/new_message/expected.txt @@ -3,7 +3,7 @@ **Effect on [astroid](https://github.com/pylint-dev/astroid):** -😞 New false positives: +New messages:
diff --git a/tests/testutils/_primer/cases/new_messages_classified/expected.txt b/tests/testutils/_primer/cases/new_messages_classified/expected.txt index cfee18bc0e..b07e280a11 100644 --- a/tests/testutils/_primer/cases/new_messages_classified/expected.txt +++ b/tests/testutils/_primer/cases/new_messages_classified/expected.txt @@ -14,20 +14,14 @@ https://github.com/astropy/astropy/blob/1fb40bc1f22f176254ef583065aa155f53f3b414/astropy/coordinates/angles/angle_parsetab.py#L14
-😞 New false positives: +New messages:
1) locally-disabled: *Locally disabling looping-through-iterator (W4801)* https://github.com/astropy/astropy/blob/1fb40bc1f22f176254ef583065aa155f53f3b414/astropy/coordinates/angles/angle_parsetab.py#L14 -
- -New messages: - -
- -1) unused-variable: +2) unused-variable: *Unused variable 'foo'* https://github.com/astropy/astropy/blob/1fb40bc1f22f176254ef583065aa155f53f3b414/astropy/coordinates/angles/angle_parsetab.py#L42
diff --git a/tests/testutils/_primer/cases/useless_suppression/expected.txt b/tests/testutils/_primer/cases/useless_suppression/expected.txt index a813b98174..5120f86953 100644 --- a/tests/testutils/_primer/cases/useless_suppression/expected.txt +++ b/tests/testutils/_primer/cases/useless_suppression/expected.txt @@ -12,7 +12,7 @@ https://github.com/astropy/astropy/blob/1fb40bc1f22f176254ef583065aa155f53f3b414/astropy/coordinates/angles/angle_parsetab.py#L14
-😞 New false positives: +New messages: