-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Issue #4551 Changed mock docks to use sphinx #4569
Conversation
docs/faq.rst
Outdated
|
||
.. Tip:: The library ``unittest.mock`` was introduced on python 3.3. On earlier versions install the ``mock`` library | ||
from PyPI with (ie ``pip install mock``) and replace the above import:: | ||
Changed in version 1.6: This config value only requires to declare the top-level modules that should be mocked. |
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.
We shouldn't put this here, instead, link to the http://www.sphinx-doc.org/en/master/usage/extensions/autodoc.html#confval-autodoc_mock_imports
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.
Ok I should just link to the document of sphinx rather than pasting it and remove the earlier workaround.
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.
@stsewd changed!
docs/faq.rst
Outdated
|
||
import sys | ||
from unittest.mock import MagicMock | ||
autodoc_mock_imports |
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.
We could start this with something more explicit, like "You can use this feature included in autodoc to mock..." where the feature name would link to the sphinx's docs. Thanks for contributing btw :)
Hey @stsewd done some changes is it ok now? |
Hey @stsewd check out the PR? |
docs/faq.rst
Outdated
@@ -65,27 +65,8 @@ I get import errors on libraries that depend on C modules | |||
|
|||
This happens because our build system doesn't have the dependencies for building your project. This happens with things like libevent and mysql, and other python things that depend on C libraries. We can't support installing random C binaries on our system, so there is another way to fix these imports. | |||
|
|||
You can mock out the imports for these modules in your ``conf.py`` with the following snippet:: | |||
You can use this feature included in autodoc to mock http://www.sphinx-doc.org/en/master/usage/extensions/autodoc.html#confval-autodoc_mock_imports. |
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.
Maybe we can put the name of the feature (and the link to it here) and end with "..to mock your dependencies."
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.
Echoing proper format for linking instead of naked link
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.
I have removed the naked link issue!
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.
Thanks for the contribution, noted a couple spots to clean up the change
docs/faq.rst
Outdated
|
||
from mock import Mock as MagicMock | ||
|
||
If such libraries are installed via ``setup.py``, you also will need to remove all the C-dependent libraries from your ``install_requires`` in the RTD environment. |
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.
We should however keep this notice, as a lot of folks have mocked their dependencies in the past, but not performed this step, assuming mocking was enough.
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.
@agjohnson , can you suggest what notice I can create for the folks?
For eg. "There is a new feature "autodock_mock_imports" for mocking the dependencies, the earlier feature present was a complex one".
Please suggest!
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.
I think this can be something like: "If you are mocking your dependencies using the mock library and such... "
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.
Now check it @stsewd !
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.
We probably just need to communicate that if you mock dependencies, in whatever fashion, that you aren't installing those dependencies either. The original copy just needs to be updated to match the new context i think.
docs/faq.rst
Outdated
@@ -65,27 +65,8 @@ I get import errors on libraries that depend on C modules | |||
|
|||
This happens because our build system doesn't have the dependencies for building your project. This happens with things like libevent and mysql, and other python things that depend on C libraries. We can't support installing random C binaries on our system, so there is another way to fix these imports. | |||
|
|||
You can mock out the imports for these modules in your ``conf.py`` with the following snippet:: | |||
You can use this feature included in autodoc to mock http://www.sphinx-doc.org/en/master/usage/extensions/autodoc.html#confval-autodoc_mock_imports. |
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.
Echoing proper format for linking instead of naked link
docs/faq.rst
Outdated
@@ -65,7 +65,7 @@ I get import errors on libraries that depend on C modules | |||
|
|||
This happens because our build system doesn't have the dependencies for building your project. This happens with things like libevent and mysql, and other python things that depend on C libraries. We can't support installing random C binaries on our system, so there is another way to fix these imports. | |||
|
|||
You can use this feature `autodoc_mock_imports`_ to mock your dependencies. | |||
If you are mocking your dependencies using the mock library, then you can also use this feature `autodoc_mock_imports`_ to mock your dependencies. |
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.
So, what Anthony mean, was to keep both paragraphs. The link to autodoc_mock_imports
and the note about the previous usage of the mock lib. Sorry about the confusion.
Now check @stsewd and @agjohnson ! |
docs/faq.rst
Outdated
@@ -87,6 +86,11 @@ Of course, replacing `MOCK_MODULES` with the modules that you want to mock out. | |||
|
|||
If such libraries are installed via ``setup.py``, you also will need to remove all the C-dependent libraries from your ``install_requires`` in the RTD environment. | |||
|
|||
If you are mocking your dependencies using the mock library, then you can also use this feature `autodoc_mock_imports`_ to mock your dependencies. |
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.
I think the autodoc feature is independent from the mock library and should be pointed first (as we want new users to use that method), the previous paragraph was ok btw, you only needed to keep the another paragraph
Check now @stsewd , I have marked the autodock feature on the top and then the previous one below. |
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.
Looks better. Just clarifying what we want to communicate to users.
docs/faq.rst
Outdated
You can mock out the imports for these modules in your ``conf.py`` with the following snippet:: | ||
If you are mocking your dependencies without using the mock library, then you can use this feature `autodoc_mock_imports`_ to mock your dependencies. | ||
|
||
.. _autodock_mock_imports: http://www.sphinx-doc.org/en/master/usage/extensions/autodoc.html#confval-autodoc_mock_imports |
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.
Link label is wrong here, should be autodoc_mock_imports
.
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.
I can't recognize, I think it's okay.
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.
There is a typo autodocK
docs/faq.rst
Outdated
@@ -65,7 +65,13 @@ I get import errors on libraries that depend on C modules | |||
|
|||
This happens because our build system doesn't have the dependencies for building your project. This happens with things like libevent and mysql, and other python things that depend on C libraries. We can't support installing random C binaries on our system, so there is another way to fix these imports. | |||
|
|||
You can mock out the imports for these modules in your ``conf.py`` with the following snippet:: | |||
If you are mocking your dependencies without using the mock library, then you can use this feature `autodoc_mock_imports`_ to mock your dependencies. |
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.
I think what we're wanting to say here is "The recommended method of mocking your libraries is to use the autodoc_mock_imports config option", not "if you are .."
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.
Yes I am changing it!
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.
@agjohnson I think its fine as it is the syntax for the removing the naked link.
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.
What Anthony says is that we should encourage people to use the autodoc_mock_imports method
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.
Pushed some small changes to get this merged. Thanks for the contribution!
Codecov Report
@@ Coverage Diff @@
## master #4569 +/- ##
=======================================
Coverage 76.33% 76.33%
=======================================
Files 158 158
Lines 9968 9968
Branches 1258 1258
=======================================
Hits 7609 7609
Misses 2018 2018
Partials 341 341 |
No description provided.