Skip to content

Conversation

Pierre-Sassoulas
Copy link
Member

Following #13727, there's some potential for match in assertrepr_compare (a lot of isinstance check are hidden inside helper function like isset isttxt, issequence, etc), and maybe some refactor to do to be able to benefit from it (in the case of set here it's to avoid creating the dict at runtime). I'm starting small, but I think it's possible to do more later.

It might be beneficial to extract function from util.py into _compare_cls.py, _compare_dict.py, _compare_iterable.py, _compare_sequence.py, _compare_set.py, _compare_str.py, assertrepr_compare.py, and format_explanation.py first, let me know if you want me to do that so that the initial match refactor PR is more focused. If I don't do that, it's better to review this PR by hiding white spaces and commit by commit.

@Pierre-Sassoulas Pierre-Sassoulas added the skip news used on prs to opt out of the changelog requirement label Sep 28, 2025
Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

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

Thanks Pierre, this seems like another good place for match-case.

I left one concern in a comment.

Regarding the factoring out of the _compare_set module, I personally prefer big modules of related functionality, it is just easier for my brain to handle. So I'm -0.5 on this, but I definitely wouldn't mind if it's split like this if you prefer it.

explanation = _compare_lt_set(left, right, highlighter, verbose)

case (
set() | frozenset(),
Copy link
Member

Choose a reason for hiding this comment

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

This duplicates the isset logic, while that seems unlikely to change it seems conceptually wrong to duplicate since the idea is to have a single place which defines what a set is. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, I have a follow up:

commit ae1fbffd1b5e7b2dc54c5cccda218f1da5173a80
Author: Pierre Sassoulas <pierre.sassoulas@gmail.com>
Date:   Sun Sep 28 07:40:28 2025 +0200

    [refactor] Group set together and two less isinstance check

diff --git a/src/_pytest/assertion/util.py b/src/_pytest/assertion/util.py
index 2086d3794..3dff746f2 100644
--- a/src/_pytest/assertion/util.py
+++ b/src/_pytest/assertion/util.py
@@ -126,10 +126,6 @@ def isdict(x: Any) -> bool:
     return isinstance(x, dict)
 
 
-def isset(x: Any) -> bool:
-    return isinstance(x, set | frozenset)
-
-
 def isnamedtuple(obj: Any) -> bool:
     return isinstance(obj, tuple) and getattr(obj, "_fields", None) is not None
 
@@ -203,18 +199,18 @@ def assertrepr_compare(
     explanation: list[str] | None
     try:
         match (left, op, right):
-            case (_, "==", _):
-                explanation = _compare_eq_any(left, right, highlighter, verbose)
-            case (str(), "not in", str()):
-                explanation = _notin_text(left, right, verbose)
             case (
                 set() | frozenset(),
-                "!=" | ">=" | "<=" | ">" | "<",
+                "==" | "!=" | ">=" | "<=" | ">" | "<",
                 set() | frozenset(),
             ):
                 explanation = SET_COMPARISON_FUNCTIONS[op](
                     left, right, highlighter, verbose
                 )
+            case (_, "==", _):
+                explanation = _compare_eq_any(left, right, highlighter, verbose)
+            case (str(), "not in", str()):
+                explanation = _notin_text(left, right, verbose)
             case _:
                 explanation = None
     except outcomes.Exit:
@@ -259,8 +255,6 @@ def _compare_eq_any(
             explanation = _compare_eq_cls(left, right, highlighter, verbose)
         elif issequence(left) and issequence(right):
             explanation = _compare_eq_sequence(left, right, highlighter, verbose)
-        elif isset(left) and isset(right):
-            explanation = _compare_eq_set(left, right, highlighter, verbose)
         elif isdict(left) and isdict(right):
             explanation = _compare_eq_dict(left, right, highlighter, verbose)

But this require a prior refactor because there's additional information that are added only for iterable in _compare_eq_any. Actually finding the right order of refactors for optimal reviewing ease was a little tricky and I decided to open this one instead of entering rebase hell.

The goal would be to remove all the isx function once the match case structure make them redundant because the complexity can be contained in the match case itself while still being readable.

Copy link
Member Author

@Pierre-Sassoulas Pierre-Sassoulas Sep 30, 2025

Choose a reason for hiding this comment

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

Also wondering if merging the entire _compare_eq_any function in the match case of assertrepr_compare is going to make it too big. But it's probably worth it because the whole logic for the assert_repr decision tree is easy to locate.

Copy link
Member

Choose a reason for hiding this comment

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

Your follow up makes sense to me (moving the type-specific checking before the generic _ == _ case).

I understand your comment about rebase hell, though it would be helpful to see the final state if you have it, even if it's one huge commit blob :)

BTW, regarding isset, I wonder if we should use collections.abc.Set (immutable set operations, includes set and frozenset, and possibly user types which implement the interface) instead of set | frozenset. I haven't checked if it makes sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

I pushed the concept a little and _compare_eq_any is recursive through _compare_eq_cls (launched on each element inside iterables) so what I envisioned is not possible. I'm going to make a second switch to match/case in _compare_eq_any and keep them separated. Here's a messy unfinished glob with what I currently have: https://github.com/Pierre-Sassoulas/pytest/pull/2/files#diff-0e1605330bae69222e30a50a4c573ae5eadb529e8f64662c0c9134e431af9d4aR27-R120

@Pierre-Sassoulas
Copy link
Member Author

Regarding the factoring out of the _compare_set module, I personally prefer big modules of related functionality, it is just easier for my brain to handle. So I'm -0.5 on this, but I definitely wouldn't mind if it's split like this if you prefer it.

I started to have "circular import" (reordering to do) inside src/_pytest/assertion/util.py when trying to create the SET_COMPARISON_FUNCTIONS dict, and it made sense that the "repr functions for a particular subset of data structure" could be separated with an explicit name, but I can also just reorder the function inside util.py if you prefer. (It's going to be as much churn, and as difficult to rebase or cherry-pick on it than creating file though)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip news used on prs to opt out of the changelog requirement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants