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 #7422: autodoc: fails with ValueError when using autodoc_mock_imports #7431

Merged

Conversation

tk0miya
Copy link
Member

@tk0miya tk0miya commented Apr 7, 2020

@stmcginnis
Copy link

Confirmed this avoids the failure encountered in the OpenStack cinder project. Thanks!

@tk0miya tk0miya changed the base branch from 3.x to 3.0.x April 7, 2020 17:10
def unwrap(obj: Any) -> Any:
"""Get an original object from wrapped object (wrapped functions)."""
try:
return inspect.unwrap(obj)
Copy link

Choose a reason for hiding this comment

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

How about providing some callback to unwrap to check if wrapped object (if there is any) id is equal to the object itself. Like (based on https://github.com/python/cpython/blob/master/Lib/inspect.py#L509):

def checker(f):
     return id(f) == id(f.__wrapped__)
inspect.unwrap(obj, stop = checker)

I guess that you may get ValueError for an object that is not mock and miss the real error.

Copy link
Contributor

Choose a reason for hiding this comment

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

@JanuszL ValueError is being thrown from https://github.com/python/cpython/blob/master/Lib/inspect.py#L524 and it's catching exactly the error you're looking for this to catch.

see: https://github.com/python/cpython/blob/master/Lib/inspect.py#L506

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, ValueError means "a cycle is encountered". It's a real error in this case. I think it is an abnormal case. So let's talk if we encountered.

Copy link

Choose a reason for hiding this comment

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

Sure. I was only thinking about the situation where the cycle is encountered and this is not due to mocking. In such a case it should really throw, so maybe there is an easier way to check if this is a moc or some valid object.

Copy link
Contributor

@terencehonles terencehonles left a comment

Choose a reason for hiding this comment

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

LGTM

return inspect.unwrap(obj)
except ValueError:
# might be a mock object
return obj
Copy link
Contributor

Choose a reason for hiding this comment

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

@tk0miya this looks good, but maybe log DEBUG/INFO?

Suggested change
return obj
logger.debug('Giving up trying to unwrap %r', obj)
return obj

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 doubt the log will be helpful. Do you want to see it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine not seeing it, I was just thinking it could be useful if you were already debugging.

@pquentin
Copy link

pquentin commented Apr 8, 2020

Thanks, this also fixes the build in urllib3.

@lpsinger
Copy link

lpsinger commented Apr 9, 2020

Thank you. This fixed the issue for my project (gwcelery).

@tk0miya tk0miya merged commit 9bb204d into sphinx-doc:3.0.x Apr 9, 2020
@tk0miya tk0miya deleted the 7422_autodoc_mock_imports_causes_ValueError branch April 9, 2020 14:44
@tk0miya
Copy link
Member Author

tk0miya commented Apr 9, 2020

Merged. Thank you for reviewing & confirming.

@kattni
Copy link

kattni commented Apr 9, 2020

@tk0miya Thank you for resolving this issue. Do you have an ETA on when a version of Sphinx including this fix will be released? I have a number of projects blocked on it.

@tk0miya
Copy link
Member Author

tk0miya commented Apr 9, 2020

@kattni Please follow #7453. I'll work for release maybe tomorrow.

@kattni
Copy link

kattni commented Apr 9, 2020

@tk0miya Thanks so much! Will do.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants