[match case] Use match case in _compare_eq_any - #14888
Open
Pierre-Sassoulas wants to merge 4 commits into
Open
Pierre-Sassoulas wants to merge 4 commits into
Pierre-Sassoulas wants to merge 4 commits into
Conversation
Pierre-Sassoulas
added
the
skip news
bluetech
approved these changes
Aug 16, 2026
bluetech
left a comment
bluetech
left a comment
Member
There was a problem hiding this comment.
Thanks.
I'm slightly torn about the last commit. It's somewhat widely agreed that str is a Sequence is a bad thing, and the pytest guards were like an attempt to fix it at the conceptual level. However, it does make things a bit harder to understand/confusing, since it diverges from Python's definition of Sequence. However2, using Python's definition introduces an ordering dependency which also makes things harder to understand.
Anyway, I'm not sure on which side I fall, so I leave it to your best judgement :)
| assertion_text_diff_style, | ||
| ) | ||
| elif issequence(left) and issequence(right): | ||
| # Stays a guard: a ``Sequence()`` pattern would also match ``str``, |
Member
There was a problem hiding this comment.
Should rephrase (or remove) the comment to not refer to what was before, which the reader shouldn't care about.
Member
Author
There was a problem hiding this comment.
Sorry about that it was fixed in the last commit but I should have amended so commit by commit review make sense.
| yield from _compare_eq_iterable( | ||
| left, right, highlighter, verbose, truncation_budget | ||
| ) | ||
| # Unreachable for two strings: ``isiterable`` is false for ``str``. |
Member
There was a problem hiding this comment.
Would write: "Note: isiterable doesn't apply to str". Or remove the comment.
| yield from right._repr_compare(left) | ||
| case (Approx(), _): | ||
| yield from left._repr_compare(right) | ||
| # ``str`` is a ``Sequence``/iterable; stop before it gets diffed per character. |
Member
There was a problem hiding this comment.
Would move this case above the Approx ones, so it's together with the str, str case. And maybe somehow combine the comment for both.
Member
Author
There was a problem hiding this comment.
If we do the str cases first "cat" == approx(3) would leave the dispatch before reaching _repr_compare and we'd lose the better approx assert message. I added a test case for this.
Pierre-Sassoulas
force-pushed
the
match-case-compare-any
branch
from
August 16, 2026 15:57
5e45f84 to
768aaa1
Compare
Replace the nested if/elif dispatch with a ``match``/``case`` on ``(left, right)``. The ``str()``, ``Approx()``, ``AbstractSet()`` and ``Mapping()`` class patterns subsume the ``istext``, ``isinstance``, ``isset`` and ``ismapping`` checks. That was the last call site of ``isset`` and ``ismapping``, so both guards are dropped here. ``istext`` stays: ``isiterable`` still uses it. Two arms stay guards on purpose: - the dataclass/attrs/namedtuple arm needs ``type(left) is type(right)``, which no pattern expresses; - ``issequence`` deliberately excludes ``str``, which a ``Sequence()`` pattern would match. Flattening the outer ``istext`` branch moves the trailing ``isiterable`` check out of the ``else``, but ``isiterable`` is false for ``str`` so the two-strings path is unaffected. ``test_exit_from_assertrepr_compare`` and ``test_exception_before_first_yield_emits_summary_and_notice`` patched ``istext`` to raise before the first yield; ``isdatacls`` is now the first module-level guard reached for ``callequal(1, 1)``. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Switching ``_compare_eq_any`` to ``str()`` class patterns left ``istext`` with a single caller: ``isiterable``, which used it to exclude strings. Inline that as ``isinstance(obj, str)`` and drop the helper. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
``str`` is both a ``Sequence`` and an iterable, so it kept leaking into arms meant for containers. Two places worked around that: the sequence arm used the ``issequence`` guard instead of a ``Sequence()`` pattern, and ``isiterable`` carried a ``not isinstance(obj, str)`` clause to keep the trailing iterable extension away from strings. Give strings their own arms instead. The text arm returns once it has produced its diff, and a following ``(str(), _) | (_, str())`` arm stops any remaining string comparison, which has no specialised explanation. Nothing below can see a ``str`` any more, so the sequence arm becomes a plain ``Sequence()`` pattern, ``issequence`` goes away, and ``isiterable`` reduces to "can you iterate it". The arm sits below the ``Approx()`` ones on purpose: ``"cat" == approx(3)`` should still get the approx explanation rather than being cut short. No behaviour change: strings already reached those checks only to be rejected by them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Move the ``approx`` cases above the ``str`` ones so both string cases sit together under a single comment, as suggested in review. ``approx`` has to stay first: it can wrap a string, and the ``str`` cases stop the dispatch, so the other order silently drops the approx explanation for ``"cat" == approx(3)``. That ordering was load bearing and untested, the approx tests call ``_repr_compare`` directly rather than going through the dispatch. Add two tests for it: one for a string compared to ``approx``, one for a string compared to another sequence, which must stay unexplained rather than being diffed per character. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Pierre-Sassoulas
force-pushed
the
match-case-compare-any
branch
from
September 6, 2026 13:27
768aaa1 to
b6dc0a0
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Use match case in _compare_eq_any and also explicitely return on string so we don't have to implicitely deal with string that are also iterable later on.