Skip to content

fix #11797: be more lenient on SequenceLike approx#11818

Merged
RonnyPfannschmidt merged 1 commit intopytest-dev:mainfrom
RonnyPfannschmidt:ronny/issue-11797-approx-sequence-like
Jun 19, 2024
Merged

fix #11797: be more lenient on SequenceLike approx#11818
RonnyPfannschmidt merged 1 commit intopytest-dev:mainfrom
RonnyPfannschmidt:ronny/issue-11797-approx-sequence-like

Conversation

@RonnyPfannschmidt
Copy link
Copy Markdown
Member

this needs a validation as it allows partially implemented sequences

closes #11797

this changes the recursive map of approx to accept all sequences (just like approx)
i would like to gather feedback form @nicoddemus and @Zac-HD on whether to accept the recursion in general,
or if we ought to limit it by changing the toplevel call in ApproxSequenceLike to a listcomp operating only on the elements of the sequence

@RonnyPfannschmidt RonnyPfannschmidt force-pushed the ronny/issue-11797-approx-sequence-like branch from 2a16b61 to 79efc62 Compare January 15, 2024 11:24
@Zac-HD
Copy link
Copy Markdown
Member

Zac-HD commented Jan 15, 2024

I'd go with the limited form only; we already treat lists and tuples as equal if their contents are approx-equal, so extending that to any sequence-like makes sense to me.

We'll just want to be careful that the repr shows the original thing, and that self-containing sequences don't cause an infinite loop (they do now for list/tuple, but adding new types makes it more likely imo).

@RonnyPfannschmidt
Copy link
Copy Markdown
Member Author

@Zac-HD the repr from ApproxSequenceLike currently shows a list of approx in the repr instead of the original object

as per

def __repr__(self) -> str:
seq_type = type(self.expected)
if seq_type not in (tuple, list):
seq_type = list
return "approx({!r})".format(
seq_type(self._approx_scalar(x) for x in self.expected)

altering that part of the repr is something i consider out of scope for this fix

@nicoddemus
Copy link
Copy Markdown
Member

nicoddemus commented Jan 16, 2024

Overall this looks good!

if we ought to limit it by changing the toplevel call in ApproxSequenceLike to a listcomp operating only on the elements of the sequence

Which top level call you mean?

@nicoddemus
Copy link
Copy Markdown
Member

altering that part of the repr is something i consider out of scope for this fix

Indeed, that's not so simple as might seem at first glance, because we cannot assume we can create arbitrary classes just by passing a sequence... a user might be using a custom class which is sequence-like, but we cannot assume we know/can create a new one from a sequence.

@RonnyPfannschmidt
Copy link
Copy Markdown
Member Author

@nicoddemus the call to the recursive map function - we have a coice between keeping certain custom types or not in its insides

im currently not sure whether the caller should map over the sequence or the recursion should expand

@thorink
Copy link
Copy Markdown

thorink commented Feb 12, 2024

Is there any update on this PR or anything I could test or where I could contribute?

Comment thread changelog/11797.bugfix.rst Outdated
@@ -0,0 +1 @@
Ensure approx for SequenceLike objects doesn't wrap those sequences in a scalar.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
Ensure approx for SequenceLike objects doesn't wrap those sequences in a scalar.
Ensure :func:`pytest.approx` for :class:`Sequence <collections.abc.Sequence>`-like objects does not wrap those sequences in a scalar.

Or:

Suggested change
Ensure approx for SequenceLike objects doesn't wrap those sequences in a scalar.
:func:`pytest.approx` now correctly handles :class:`Sequence <collections.abc.Sequence>`-like objects.

But then this feels more like an improvement than a bugfix? 🤔

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'll recategorize and rebase as soon as I get to it again

@RonnyPfannschmidt RonnyPfannschmidt force-pushed the ronny/issue-11797-approx-sequence-like branch from a3a2ffc to c8aa13c Compare June 18, 2024 11:26
@RonnyPfannschmidt RonnyPfannschmidt self-assigned this Jun 18, 2024
this needs a validation as it allows partially implemented sequences
@RonnyPfannschmidt RonnyPfannschmidt force-pushed the ronny/issue-11797-approx-sequence-like branch from 3a2d0bd to 76f3f3d Compare June 18, 2024 15:01
@RonnyPfannschmidt RonnyPfannschmidt merged commit 08a39bf into pytest-dev:main Jun 19, 2024
@RonnyPfannschmidt RonnyPfannschmidt deleted the ronny/issue-11797-approx-sequence-like branch June 19, 2024 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

approx representation of details failed when using a ApproxSequenceLike which is not list or tuple

4 participants