Fix issue with :skip: introduced by :include: feature #142

adrn · 2021-12-28T23:13:39Z

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:

adrn · 2021-12-28T23:19:42Z

Uhh, what's up with the failing tests?? Any ideas @pllim?

pllim · 2021-12-29T14:24:03Z

Looks like Sphinx is using distutils but it is deprecated now. I guess we can ignore it...

../../.tox/py38-test-sphinx35/lib/python3.8/site-packages/sphinx/util/smartypants.py:33: in <module>
    from sphinx.util.docutils import __version_info__ as docutils_version
../../.tox/py38-test-sphinx35/lib/python3.8/site-packages/sphinx/util/docutils.py:45: in <module>
    __version_info__ = tuple(LooseVersion(docutils.__version__).version)
../../.tox/py38-test-sphinx35/lib/python3.8/site-packages/setuptools/_distutils/version.py:53: in __init__
    warnings.warn(
E   DeprecationWarning: distutils Version classes are deprecated. Use packaging.version instead.

codecov · 2021-12-29T14:27:16Z

Codecov Report

Merging #142 (a9177d4) into main (ef2e5dd) will increase coverage by 0.14%.
The diff coverage is 96.15%.

@@            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

Impacted Files	Coverage Δ
sphinx_automodapi/tests/example_module/stdlib.py	`80.00% <80.00%> (ø)`
sphinx_automodapi/automodapi.py	`93.29% <100.00%> (+0.04%)`	⬆️
sphinx_automodapi/tests/test_automodapi.py	`99.16% <100.00%> (+0.14%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bcc41ff...a9177d4. Read the comment docs.

pllim · 2021-12-29T14:29:11Z

I don't grok what is going on here, so I'll wait for @eslavich to review. Thanks for the quick fix!

eslavich

One possible issue, thanks for the fast turnaround on this @adrn!

eslavich · 2021-12-29T17:20:46Z

sphinx_automodapi/automodapi.py

@@ -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)):


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 ```

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.

Ah, okay! That makes sense.

Do you at least want to put this test in CI before merge?

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.

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!

eslavich

LGTM

pllim

Thanks, all!

pllim · 2021-12-29T18:05:16Z

Since I also fixed jwst docs to not require this patch, I don't think an immediate release is needed. But please let me know if you disagree. Thanks!

mwcraig · 2022-01-12T17:43:12Z

@pllim -- if you have a chance a new release would be helpful because ccdproc tests are failing because of this (we have defined __all__ so that fix won't work here).

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

pllim · 2022-01-12T18:32:26Z

Done. v0.14.1 is on PyPI. Hope this helps!

Ed Slavich and others added 4 commits December 27, 2021 14:10

Add test of :skip: with stdlib objects

baa95e9

Fix style issue

8be51e3

fix for skip bug

610bb35

add note about regression test

57a8360

adrn force-pushed the fix-skip-issue branch from f75b44d to 57a8360 Compare December 28, 2021 23:17

adrn mentioned this pull request Dec 28, 2021

:skip: no longer works with standard library objects #141

Closed

Ignore distutils DeprecationWarning from Sphinx

4ae1ae4

pllim added the bug label Dec 29, 2021

pllim added this to the 0.15.0 milestone Dec 29, 2021

Update CHANGES.rst

b355d30

eslavich reviewed Dec 29, 2021

View reviewed changes

eslavich approved these changes Dec 29, 2021

View reviewed changes

add one more validation that non-local names get skipped

a9177d4

pllim approved these changes Dec 29, 2021

View reviewed changes

pllim merged commit 3c9c885 into astropy:main Dec 29, 2021

pllim modified the milestones: 0.15.0, 0.14.1 Jan 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue with :skip: introduced by :include: feature #142

Fix issue with :skip: introduced by :include: feature #142

adrn commented Dec 28, 2021 •

edited by pllim

adrn commented Dec 28, 2021

pllim commented Dec 29, 2021

codecov bot commented Dec 29, 2021 •

edited

pllim commented Dec 29, 2021

eslavich left a comment

eslavich Dec 29, 2021

adrn Dec 29, 2021 •

edited

eslavich Dec 29, 2021

pllim Dec 29, 2021

adrn Dec 29, 2021

eslavich Dec 29, 2021

adrn Dec 29, 2021

eslavich left a comment

pllim left a comment

pllim commented Dec 29, 2021

mwcraig commented Jan 12, 2022

pllim commented Jan 12, 2022

Fix issue with :skip: introduced by :include: feature #142

Fix issue with :skip: introduced by :include: feature #142

Conversation

adrn commented Dec 28, 2021 • edited by pllim

adrn commented Dec 28, 2021

pllim commented Dec 29, 2021

codecov bot commented Dec 29, 2021 • edited

Codecov Report

pllim commented Dec 29, 2021

eslavich left a comment

Choose a reason for hiding this comment

eslavich Dec 29, 2021

Choose a reason for hiding this comment

adrn Dec 29, 2021 • edited

Choose a reason for hiding this comment

eslavich Dec 29, 2021

Choose a reason for hiding this comment

pllim Dec 29, 2021

Choose a reason for hiding this comment

adrn Dec 29, 2021

Choose a reason for hiding this comment

eslavich Dec 29, 2021

Choose a reason for hiding this comment

adrn Dec 29, 2021

Choose a reason for hiding this comment

eslavich left a comment

Choose a reason for hiding this comment

pllim left a comment

Choose a reason for hiding this comment

pllim commented Dec 29, 2021

mwcraig commented Jan 12, 2022

pllim commented Jan 12, 2022

adrn commented Dec 28, 2021 •

edited by pllim

codecov bot commented Dec 29, 2021 •

edited

adrn Dec 29, 2021 •

edited