-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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 doctest collection of functools.cached_property
objects.
#11317
Conversation
|
||
# Type ignored because this is a private function. | ||
return super()._from_module(module, object) # type: ignore[misc] | ||
|
||
if self.path.name == "conftest.py": |
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.
At first glance this Looks like this ought to be a extra elif next two the property handling of the mock class with the hack's
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.
Yeah that makes perfect sense. I'm going to wait on the cpython fix to be implemented before pushing this forward. It might offer some additional insights.
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.
python/cpython#107996 has been merged. I think the original idea of the PR is still the way to fix this issue. However, I did move the method override of _from_module
to the pre-existing DocTestFinder
subclass.
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.
Should we rename MockAwareDocTestFinder
?
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.
LGTM! I'll let @RonnyPfannschmidt do the merge.
Should we backport this? |
I'm under the impression we should |
Can you review/merge and backport then @RonnyPfannschmidt ? Or would you like me to go ahead with it? |
I won't be able to get to it before next week earliest |
@nicoddemus any idea why the back-porting workflow failed? |
We need to add the |
i see, can we also trigger it on a merge if there is the label applied? |
We did consider it at the time, but IIRC it would complicate the action significantly and nobody wanted to tackle that. |
I'll be first to admit that the proposed solution is very hacky but felt inspired by some adjacent code to do things this way. Suggestions for improvement are welcomed. 🙂
closes #11237