Skip to content

[primer] Show changed messages as diffs instead of removed+added#10914

Draft
Pierre-Sassoulas wants to merge 8 commits intopylint-dev:mainfrom
Pierre-Sassoulas:primer-message-smarter-diff
Draft

[primer] Show changed messages as diffs instead of removed+added#10914
Pierre-Sassoulas wants to merge 8 commits intopylint-dev:mainfrom
Pierre-Sassoulas:primer-message-smarter-diff

Conversation

@Pierre-Sassoulas
Copy link
Copy Markdown
Member

@Pierre-Sassoulas Pierre-Sassoulas commented Mar 10, 2026

Type of Changes

Type
✨ New feature

Description

Instead of reporting moved or reworded diagnostics as separate removal + addition, the primer comparator now pairs residual messages by (symbol, path, obj) and shows a compact diff of what changed.

Rendering improvements:

  • Symbol name is a clickable link to the source location
  • Location changes shown with backtick-formatted spans (100:0 to 100:15)
  • ^^ caret hints highlight exactly which characters changed in the message text
  • Blank line before ```diff blocks so GitHub renders them correctly
  • Extract _details_section helper to deduplicate
    Details wrapping

Test infrastructure:

  • Move test data from cases/ and batched_cases/ to fixtures/
  • Add fixtures: line_moved, message_changed_line, mixed_changes, type_changed
  • Add test_truncated_compare_in_details for truncation inside a
    Details block

@Pierre-Sassoulas Pierre-Sassoulas added Maintenance Discussion or action around maintaining pylint or the dev workflow Skip news 🔇 This change does not require a changelog entry primer labels Mar 10, 2026
@github-actions

This comment has been minimized.

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 10, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.22%. Comparing base (b080a21) to head (47eda50).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #10914      +/-   ##
==========================================
+ Coverage   96.19%   96.22%   +0.02%     
==========================================
  Files         178      178              
  Lines       19683    19778      +95     
==========================================
+ Hits        18934    19031      +97     
+ Misses        749      747       -2     
Files with missing lines Coverage Δ
pylint/testutils/_primer/comparator.py 100.00% <100.00%> (ø)
pylint/testutils/_primer/primer_compare_command.py 100.00% <100.00%> (+2.73%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Collaborator

@DanielNoord DanielNoord left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really better? It seems like a lot of code for very minimal gain?

Perhaps better suggestion would be to show the changes based on position instead of message name? That should catch most "message was slightly changed" cases in a simple overview without the fuzzy matching being necessary

@Pierre-Sassoulas
Copy link
Copy Markdown
Member Author

Right, I'm going to reuse the comparator PR from earlier too.

@github-actions

This comment has been minimized.

@Pierre-Sassoulas Pierre-Sassoulas force-pushed the primer-message-smarter-diff branch from c819028 to c5c3e01 Compare March 22, 2026 14:48
@github-actions

This comment has been minimized.

@Pierre-Sassoulas Pierre-Sassoulas force-pushed the primer-message-smarter-diff branch 2 times, most recently from ef031be to 3edeef0 Compare March 31, 2026 20:30
@Pierre-Sassoulas Pierre-Sassoulas marked this pull request as draft March 31, 2026 20:30
@Pierre-Sassoulas Pierre-Sassoulas force-pushed the primer-message-smarter-diff branch from 3edeef0 to 80285ff Compare April 2, 2026 19:39
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@Pierre-Sassoulas Pierre-Sassoulas force-pushed the primer-message-smarter-diff branch from 995984c to f741439 Compare April 3, 2026 06:12
@github-actions

This comment has been minimized.

@Pierre-Sassoulas Pierre-Sassoulas force-pushed the primer-message-smarter-diff branch from f741439 to 546b930 Compare April 18, 2026 12:34
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

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.
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 <details> blocks with the blank line
required for GitHub markdown rendering.
- 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.
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.
… helpers

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.
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.
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).
@Pierre-Sassoulas Pierre-Sassoulas force-pushed the primer-message-smarter-diff branch from f58d4c5 to 6e7d3c9 Compare April 26, 2026 12:10
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.
@github-actions
Copy link
Copy Markdown
Contributor

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit 47eda50

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Maintenance Discussion or action around maintaining pylint or the dev workflow primer Skip news 🔇 This change does not require a changelog entry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants