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 issue with :skip: introduced by :include: feature #142
Conversation
Uhh, what's up with the failing tests?? Any ideas @pllim? |
Looks like Sphinx is using distutils but it is deprecated now. I guess we can ignore it...
|
Codecov Report
@@ Coverage Diff @@
## main #142 +/- ##
==========================================
+ Coverage 89.00% 89.14% +0.14%
==========================================
Files 22 23 +1
Lines 1091 1115 +24
==========================================
+ Hits 971 994 +23
- Misses 120 121 +1
Continue to review full report at Codecov.
|
I don't grok what is going on here, so I'll wait for @eslavich to review. Thanks for the quick fix! |
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.
One possible issue, thanks for the fast turnaround on this @adrn!
@@ -411,12 +411,12 @@ def _mod_info(modname, toskip=[], include=[], onlylocals=True): | |||
|
|||
hascls = hasfunc = hasother = False | |||
|
|||
skips = [] | |||
skips = toskip.copy() | |||
for localnm, fqnm, obj in zip(*find_mod_objs(modname, onlylocals=onlylocals)): |
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 new :include:
feature has a similar problem as was introduced for :skip:
. Here's a test that fails on this branch:
am_replacer_include_stdlib_str = """
This comes before
.. automodapi:: sphinx_automodapi.tests.example_module.stdlib
:include: add
This comes after
"""
am_replacer_include_stdlib_expected = """
This comes before
sphinx_automodapi.tests.example_module.stdlib Module
----------------------------------------------------
.. automodule:: sphinx_automodapi.tests.example_module.stdlib
Functions
^^^^^^^^^
.. automodsumm:: sphinx_automodapi.tests.example_module.stdlib
:functions-only:
:toctree: api
:skip: Path,time
This comes after
""".format(empty='')
def test_am_replacer_include_stdlib(tmpdir):
"""
Tests using the ":include: option in an ".. automodapi::"
in the presence of objects imported from the standard library.
"""
with open(tmpdir.join('index.rst').strpath, 'w') as f:
f.write(am_replacer_include_stdlib_str.format(options=''))
run_sphinx_in_tmpdir(tmpdir)
with open(tmpdir.join('index.rst.automodapi').strpath) as f:
result = f.read()
assert result == am_replacer_include_stdlib_expected
```
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 don't think this is an issue -- the function names would only need to be skipped if you added the stdlib module names to :allowed-package-names:
. For example,
am_replacer_include_stdlib_str = """
This comes before
.. automodapi:: sphinx_automodapi.tests.example_module.stdlib
:include: add
:allowed-package-names: pathlib, datetime, sphinx_automodapi
This comes after
"""
am_replacer_include_stdlib_expected = """
This comes before
sphinx_automodapi.tests.example_module.stdlib Module
----------------------------------------------------
.. automodule:: sphinx_automodapi.tests.example_module.stdlib
Functions
^^^^^^^^^
.. automodsumm:: sphinx_automodapi.tests.example_module.stdlib
:functions-only:
:toctree: api
:skip: Path,time
:allowed-package-names: pathlib,datetime,sphinx_automodapi
This comes after
""".format(empty='')
Then the test passes. Otherwise, find_mod_objs()
has onlylocals=True
and ignores the imported names.
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.
Ah, okay! That makes sense.
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.
Do you at least want to put this test in CI before merge?
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.
Sure
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'm not sure it's a worthy test, given what @adrn told me -- it makes sense that :skip:
doesn't appear, but I'm not sure that we need to assert that it is not present.
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 added one more check that doing what I said in the comment above makes your test pass -- I think that is still worth including!
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
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, all!
Since I also fixed |
@pllim -- if you have a chance a new release would be helpful because Failing test: https://github.com/astropy/ccdproc/runs/4757634889?check_suite_focus=true#step:7:79 Pinned the version for now, so no an emergency |
Done. v0.14.1 is on PyPI. Hope this helps! |
This is a fix for the bug identified in #141. The fix is implemented in the first commit from me, which simplifies some of the logic and reverts back to looking more like it did before #127 but keeps the new
:include:
functionality.Note: this includes the commits from #140 which added a useful regression test.
cc @eslavich
EDIT: