Skip to content
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

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

RonnyPfannschmidt
Copy link
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

this needs a validation as it allows partially implemented sequences
@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
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
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
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
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
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

thorink commented Feb 12, 2024

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

@@ -0,0 +1 @@
Ensure approx for SequenceLike objects doesn't wrap those sequences in a scalar.
Copy link
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
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
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