Fix issue with :skip: introduced by :include: feature by adrn · Pull Request #142 · astropy/sphinx-automodapi

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


-    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

Uh oh!

Conversation

adrn commented Dec 28, 2021 • edited by pllim Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adrn commented Dec 28, 2021

Uh oh!

pllim commented Dec 29, 2021

Uh oh!

codecov bot commented Dec 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pllim commented Dec 29, 2021

Uh oh!

eslavich left a comment

Choose a reason for hiding this comment

Uh oh!

eslavich Dec 29, 2021

Choose a reason for hiding this comment

Uh oh!

adrn Dec 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eslavich Dec 29, 2021

Choose a reason for hiding this comment

Uh oh!

pllim Dec 29, 2021

Choose a reason for hiding this comment

Uh oh!

adrn Dec 29, 2021

Choose a reason for hiding this comment

Uh oh!

eslavich Dec 29, 2021

Choose a reason for hiding this comment

Uh oh!

adrn Dec 29, 2021

Choose a reason for hiding this comment

Uh oh!

eslavich left a comment

Choose a reason for hiding this comment

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

pllim commented Dec 29, 2021

Uh oh!

mwcraig commented Jan 12, 2022

Uh oh!

pllim commented Jan 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

adrn commented Dec 28, 2021 •

edited by pllim

Loading

codecov bot commented Dec 29, 2021 •

edited

Loading

adrn Dec 29, 2021 •

edited

Loading