-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Sphinx crash: documentation config fix #35763
Sphinx crash: documentation config fix #35763
Conversation
@cachedout I just stepped on the rake that our package does not generate
What you think? You removed these modules to prevent crash, but this is "shaky" in terms of I would rather keep them mocked properly. |
@whiteinge and @jacobhammons you'll want to know about this also |
It's a good fix. I vote we merge this now and explore the salt-deps alternative to mocking discussed in #35729 (comment) as a separate experiment off the |
Thanks @isbm! We were discussing adding psutil back in internally the other day, so this is good. Thanks for looking @whiteinge. |
I explored this approach already and rejected it for the reasons I outlined here: Personally, I don't think this is going to be sustainable but we can go with it for now and see how things turn out. :] |
@cachedout But now you can do: mapping = {'this_returns_tuple': tuple(),
'this_returns_function': lambda x=None:"Hi there",
'this_returns_an_int': 0}
mock = Mock(mapping) This would invalidate your 1st argument. I have an ideas how to return multiple types of values. But that sucks by a design that the function can return different types, at first place... |
In your example |
Right (at each function). Problem is that you cannot mock everything at one go, unfortunately. Some values are expected back and if they don't appear as such, we have a problem. I would see two options here:
First approach will require some maintenance of the list, but will make sure docs are within the integrity. |
I agree that it's manageable at the present time. My concern is about the future. But, as I mentioned earlier, I do think the way to go is to keep this change in and revisit it down the road if it does end up becoming a problem. :] |
Well, as of manageability, it doesn't have to be all fat in the # Module description
module = {'method': value}
# Module description
module1 = {'method': value}
... It looks quite manageable to me even for a big amount of stuff. Problem is that for now I don't see anything else non-intrusive, unless I miss something. 😏 Changing Salt? Not at all. Changing Sphinx? Not either, because the thing needs to know the result, skipping not an option. At the same time I do not expect we mocking everything. It unlikely will be that big. |
I agree with both of you. ^_^ Mocking is manageable if it's our only option. But it is a time-sink and sometimes tricky to debug so if we can pull it out all the better. Issue filed here to experiment with an alternative: #35883. |
😆 |
FWIW, the docs are broken again.
|
@cachedout hmm, can't reproduce this. Used Sphinx 1.4.3, noticed you used 1.4.5, upgraded to the latest 1.4.6 and still works. Successfully built the docs. As I think that the |
Stupid me. Because I have psutil... |
OK, disabled |
@isbm Hmm, OK. Let me poke around and see if I can figure out what's causing my local build to fail. Thanks for having a look here. |
What does this PR do?
Fixes sphinx doc generator on mocked imports.
What issues does this PR fix or reference?
Previous Behavior
New Behavior
Problem gone.
Tests written?
No