-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
[refactor] Start using match case in 'assertrepr_compare' #13762
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[refactor] Start using match case in 'assertrepr_compare' #13762
Conversation
Instead of creating it at runtime all the time
ac60fb8
to
e35f1d9
Compare
There was a problem hiding this 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(), |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
I started to have "circular import" (reordering to do) inside |
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.