Use node.findall if available (docutils 18.x) #403

drammock · 2022-05-28T19:34:43Z

closes #399

jarrodmillman · 2022-05-28T20:02:57Z

You can fix the linter warnings via:

pip install pre-commit
pre-commit run --all-files --show-diff-on-failure --color always

Or do

 pip install pre-commit
 pre-commit install

and it will automatically run when you try to commit.

jarrodmillman · 2022-05-28T20:05:27Z

We will want to make sure we still support docutils < 0.18 and sphinx==4, I think.

drammock · 2022-05-28T20:06:38Z

pre-commit is failing, looks like it mistakes python 3.10 for python 3.1 (!!) here's the log

version information

pre-commit version: 2.19.0
git --version: git version 2.25.1
sys.version:
    3.10.4 | packaged by conda-forge | (main, Mar 24 2022, 17:39:04) [GCC 10.3.0]
sys.executable: /opt/miniforge3/envs/numpydoc-dev/bin/python3.1
os.name: posix
sys.platform: linux

error information

An unexpected error has occurred: CalledProcessError: command: ('/opt/miniforge3/envs/numpydoc-dev/bin/python3.1', '-mvirtualenv', '/home/drmccloy/.cache/pre-commit/repowbc6hl47/py_env-python3.1', '-p', 'python3.1')
return code: 1
expected return code: 0
stdout:
    RuntimeError: failed to find interpreter for Builtin discover of python_spec='python3.1'
    
stderr: (none)

Traceback (most recent call last):
  File "/opt/miniforge3/envs/numpydoc-dev/lib/python3.10/site-packages/pre_commit/error_handler.py", line 73, in error_handler
    yield
  File "/opt/miniforge3/envs/numpydoc-dev/lib/python3.10/site-packages/pre_commit/main.py", line 361, in main
    return hook_impl(
  File "/opt/miniforge3/envs/numpydoc-dev/lib/python3.10/site-packages/pre_commit/commands/hook_impl.py", line 238, in hook_impl
    return retv | run(config, store, ns)
  File "/opt/miniforge3/envs/numpydoc-dev/lib/python3.10/site-packages/pre_commit/commands/run.py", line 414, in run
    install_hook_envs(to_install, store)
  File "/opt/miniforge3/envs/numpydoc-dev/lib/python3.10/site-packages/pre_commit/repository.py", line 223, in install_hook_envs
    _hook_install(hook)
  File "/opt/miniforge3/envs/numpydoc-dev/lib/python3.10/site-packages/pre_commit/repository.py", line 79, in _hook_install
    lang.install_environment(
  File "/opt/miniforge3/envs/numpydoc-dev/lib/python3.10/site-packages/pre_commit/languages/python.py", line 219, in install_environment
    cmd_output_b(*venv_cmd, cwd='/')
  File "/opt/miniforge3/envs/numpydoc-dev/lib/python3.10/site-packages/pre_commit/util.py", line 146, in cmd_output_b
    raise CalledProcessError(returncode, cmd, retcode, stdout_b, stderr_b)
pre_commit.util.CalledProcessError: command: ('/opt/miniforge3/envs/numpydoc-dev/bin/python3.1', '-mvirtualenv', '/home/drmccloy/.cache/pre-commit/repowbc6hl47/py_env-python3.1', '-p', 'python3.1')
return code: 1
expected return code: 0
stdout:
    RuntimeError: failed to find interpreter for Builtin discover of python_spec='python3.1'
    
stderr: (none)

EDIT fixed by manually typing the zero into .git/hooks/pre-commit 🤦🏻

drammock · 2022-05-28T20:16:07Z

We will want to make sure we still support docutils < 0.18 and sphinx==4, I think.

my change to test_reference will break that, I think. Stand by.

numpydoc/numpydoc.py

numpydoc/tests/test_full.py

Co-authored-by: Jarrod Millman <[email protected]>

rossbar · 2022-05-31T23:16:02Z

numpydoc/numpydoc.py

+    """Triage node.traverse (docutils <0.18) vs node.findall.
+
+    TODO: This check can be removed when the minimum supported docutils version
+    for numpydoc is docutils>=0.18


Just to be clear, the findall method was only added in docutils 0.18, correct?

I double-checked, and in fact it wasn't introduced until 0.18.1:

Release 0.18.1 ============== * docutils/nodes.py - `Node.traverse()` returns a list again to restore backwards compatibility. Fixes bug #431. - New method `Node.findall()`: like `Node.traverse()` but returns an iterator. Obsoletes `Node.traverse()`.

(from https://sourceforge.net/p/docutils/code/HEAD/tree/trunk/docutils/HISTORY.txt)

I'll push a commit to be more specific about the version.

numpydoc/tests/test_full.py

drammock · 2022-06-01T14:47:14Z

Just a note that although the CIs were coming back green before the last commit, I don't think they're actually testing against sphinx 5.0beta / docutils 0.18 ? Locally I still see this:

=================================== short test summary info ===================================
XFAIL numpydoc/tests/test_validate.py::TestValidator::test_bad_docstrings[BadParameters-blank_lines-msgs30]
XFAIL numpydoc/tests/test_validate.py::TestValidator::test_bad_docstrings[BadReturns-no_type-msgs33]
=============================== 237 passed, 2 xfailed in 1.73s ================================

and I'm not sure how to fix it... that pytest.parametrize is over 200 lines long and I haven't had the time to wrap my head around it. Maybe @datapythonista or @larsoner have time to look?

drammock · 2022-06-01T15:00:53Z

Just a note that although the CIs were coming back green before the last commit, I don't think they're actually testing against sphinx 5.0beta / docutils 0.18 ?

Never mind, I'm wrong, I just spotted the prerelease jobs. And I also just realized that I was mis-reading the pytest output. Those are successfully failing XFAILs not unexpectedly passing XFAILs. So I think this is ready for review/merge @jarrodmillman

larsoner

Code changes LGTM, +1 for merge. Feel free to merge if you're happy @jarrodmillman !

I can also confirm that on docutils 0.18.1 I have failures building MNE-Python's doc, and with this PR things build fine.

jarrodmillman · 2022-06-02T00:31:40Z

See #404. That should fix the test failures.

drammock · 2022-06-02T11:07:13Z

See #404. That should fix the test failures.

Ok great! Can you manually re-trigger the failing runs? I can't just push an empty commit; as a first time contrib here, the CIs will wait for maintainer approval anyway.

larsoner · 2022-06-02T11:10:35Z

Since it's a change to the CI yaml you'll need to merge or rebase I think

drammock · 2022-06-02T11:15:09Z

Since it's a change to the CI yaml you'll need to merge or rebase I think

Ah right, of course. Will do when I'm not on a phone

larsoner · 2022-06-02T15:55:20Z

All green!

jarrodmillman · 2022-06-02T16:03:11Z

Thanks!!

drammock mentioned this pull request May 28, 2022

sphinx 5 compatibility #399

Closed

drammock added 2 commits May 28, 2022 15:13

use node.findall if available (docutils 18.x)

76c033b

fix test_reference

29caa49

drammock force-pushed the node-traverse branch from 84d8512 to 29caa49 Compare May 28, 2022 20:14

keep test_reference backwards-compatible

61becba

jarrodmillman changed the title ~~use node.findall if available (docutils 18.x)~~ Use node.findall if available (docutils 18.x) May 31, 2022

jarrodmillman added this to the 1.4.0 milestone May 31, 2022

jarrodmillman requested changes May 31, 2022

View reviewed changes

numpydoc/numpydoc.py Outdated Show resolved Hide resolved

numpydoc/tests/test_full.py Show resolved Hide resolved

jarrodmillman marked this pull request as ready for review May 31, 2022 17:47

drammock and others added 2 commits May 31, 2022 13:24

Add comments about when triage becomes unnecessary/obsolete

99d0961

Co-authored-by: Jarrod Millman <[email protected]>

whitespace

0df45f0

rossbar reviewed May 31, 2022

View reviewed changes

drammock commented Jun 1, 2022

View reviewed changes

numpydoc/tests/test_full.py Show resolved Hide resolved

fix docutils version as noted in docstring

296bbde

larsoner approved these changes Jun 1, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/main' into node-traverse

04c812d

jarrodmillman approved these changes Jun 2, 2022

View reviewed changes

jarrodmillman merged commit 601642b into numpy:main Jun 2, 2022

jarrodmillman mentioned this pull request Jun 2, 2022

Test docutils 0.18.1 #402

Merged

This was referenced Jun 2, 2022

Future: _traverse_or_findall -> findall when docutils 0.18 is the minimum version #405

Open

1.4.0 release plan #408

Closed

drammock mentioned this pull request Jun 24, 2022

[MAINT, MRG] Fix nilearn intersphinx mne-tools/mne-python#10825

Merged

drammock deleted the node-traverse branch June 24, 2022 21:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use node.findall if available (docutils 18.x) #403

Use node.findall if available (docutils 18.x) #403

drammock commented May 28, 2022 •

edited

Loading

jarrodmillman commented May 28, 2022

jarrodmillman commented May 28, 2022

drammock commented May 28, 2022 •

edited

Loading

version information

error information

drammock commented May 28, 2022

rossbar May 31, 2022

drammock Jun 1, 2022

drammock commented Jun 1, 2022

drammock commented Jun 1, 2022

larsoner left a comment

jarrodmillman commented Jun 2, 2022

drammock commented Jun 2, 2022

larsoner commented Jun 2, 2022

drammock commented Jun 2, 2022

larsoner commented Jun 2, 2022

jarrodmillman commented Jun 2, 2022

Use node.findall if available (docutils 18.x) #403

Use node.findall if available (docutils 18.x) #403

Conversation

drammock commented May 28, 2022 • edited Loading

jarrodmillman commented May 28, 2022

jarrodmillman commented May 28, 2022

drammock commented May 28, 2022 • edited Loading

version information

error information

drammock commented May 28, 2022

rossbar May 31, 2022

Choose a reason for hiding this comment

drammock Jun 1, 2022

Choose a reason for hiding this comment

drammock commented Jun 1, 2022

drammock commented Jun 1, 2022

larsoner left a comment

Choose a reason for hiding this comment

jarrodmillman commented Jun 2, 2022

drammock commented Jun 2, 2022

larsoner commented Jun 2, 2022

drammock commented Jun 2, 2022

larsoner commented Jun 2, 2022

jarrodmillman commented Jun 2, 2022

drammock commented May 28, 2022 •

edited

Loading

drammock commented May 28, 2022 •

edited

Loading