Skip to content
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

TST: run refguide-check on rst files in doc/* #14732

Merged
merged 7 commits into from Nov 23, 2019
Merged

Conversation

mattip
Copy link
Member

@mattip mattip commented Oct 17, 2019

xref #14723

Run refguide-check on files under doc, skip the 'scipy-sphinx-theme', 'sphinxext', 'neps', and 'release' subdirectories for now.

Changes:

  • Add a --rst option to refguide-check.py to dive into the doc directory and implement it
  • Fix up line numbers in error reporting
  • Clean up some of the comments in refguide-check

Also

  • Skip running all the tests in the doctests in doc/source/dev/development_environment.rst

@mattip
Copy link
Member Author

mattip commented Oct 17, 2019

The test-run failure demonstrates that the files are being checked :)

@mattip
Copy link
Member Author

mattip commented Oct 17, 2019

Now that the log files of the refguide-check run prove this is working, I would like to remove the check from CI and get this merged. The next step would be to encourage others to work on fixing all the failures, and then eventually enable this in CI again when it is clean.

@eric-wieser
Copy link
Member

nit: TST not TEST I think is our usual prefix

@mattip
Copy link
Member Author

mattip commented Oct 17, 2019

squashed and force pushed

  • add a SKIPBLOCK option, but note it must be used on a contiguous block, no blank lines
  • clean up some reverted changes to files using the new SKIPBLOCK option
  • add the option to skip a file to the already existing option to skip a single directory or an entire tree

@bsipocz
Copy link
Member

bsipocz commented Oct 17, 2019

I plan to have a review of this, but for now just a quick question:

add a SKIPBLOCK option, but note it must be used on a contiguous block, no blank lines

have you had a look into pytest-doctestplus? it already has a few directives to skip doctesting certain parts

@mattip
Copy link
Member Author

mattip commented Oct 17, 2019

I don't see a SKIPBLOCK directive. It would need to be on the level of a doctest runner, since at the level of the checker all you have is a single example in a block, not the entire block of examples.

We started with refguide in gh-9415 since it was felt doctest would never be up to the task at hand.

@bsipocz
Copy link
Member

bsipocz commented Oct 17, 2019

Isn't this the use case you're after?: https://github.com/astropy/astropy/blame/master/docs/nddata/utils.rst#L40

@mattip
Copy link
Member Author

mattip commented Oct 17, 2019

yes, except the .. doctest-skip:: RST directive doesn't work with refguide (I just tried). Is that supposed to be respected by doctests or is a part of sphinx?

@bsipocz
Copy link
Member

bsipocz commented Oct 17, 2019

It's a plugin for pytest, so should be part of doctesting, sphinx doesn't do any code checking. For the bare minimum, you need doctest_plus = enabled in pytest.ini, but I haven't yet get it work with your testing infrastructure either.

And it also works for any text file format, I used it extensively for tex files to make sure code examples in a book manuscript work at least at the time sending it to the publisher :)

@mattip
Copy link
Member Author

mattip commented Oct 17, 2019

@bsipocz if I understand you correctly doctest_plus seems to be a totally different framework for docstring testing, so probably needs to be a separate PR/discussion on the mailing list.

@bsipocz
Copy link
Member

bsipocz commented Oct 17, 2019

I don't disagree about the mailing list, but what do you mean by totally different framework? It simply extends the standard doctest functionalities.

@mattip
Copy link
Member Author

mattip commented Oct 17, 2019

We are using tool/refguide_check.py to do docstring testing. It is not based on pytest, rather is a stand-alone tool that extends the stdlib doctests directly. I don't think we can make it part of a pytest workflow.

@rgommers rgommers changed the title TEST: run refguide-check on rst files in doc/* TST: run refguide-check on rst files in doc/* Oct 18, 2019
@mattip
Copy link
Member Author

mattip commented Nov 4, 2019

Closes #14707

@WarrenWeckesser
Copy link
Member

@mattip, overall this looks fine. There are a couple unresolved comments from @eric-wieser. I requested more information for one of them.

@mattip
Copy link
Member Author

mattip commented Nov 14, 2019

The test failure is due to a bad install of mingw32 and not connected to this PR.

Copy link
Member

@WarrenWeckesser WarrenWeckesser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I'll hold off merging in case @eric-wieser has any additional comments on the last updates.

def check_items(all_dict, names, deprecated, others, module_name, dots=True):
"""Check that `all_dict` is consistent with the `names` in `module_name`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't follow the docstring standard.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

'Inf': np.inf,}
'Inf': np.inf,
'StringIO': io.StringIO,
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation looks off.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved to column 1



def check_documentation(base_path, results, args, dots):
"""Check examples in any *.rst located inside `base_path`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs standard docstring.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed formatting

tools/refguide_check.py Outdated Show resolved Hide resolved
sys.stderr.write(filename + ' ')
sys.stderr.flush()

tut_results = check_doctests_testfile(filename,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linebreak after (.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

for v in mod_results:
assert isinstance(v, tuple), v
mod_results = []
mod_results += check_items(all_dict, names, deprecated, others, module.__name__)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line too long.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@charris
Copy link
Member

charris commented Nov 17, 2019

Style nits.

@mattip
Copy link
Member Author

mattip commented Nov 18, 2019

style nits corrected

Copy link
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for the persistence.

I think my main confusion was over running the rst checker over python files - it all makes sense to me now we're not doing that. If it turns out I'm wrong and it would make sense to do that, we can follow up in a later PR.

@charris
Copy link
Member

charris commented Nov 19, 2019

The docstrings are still not following the docstring standard https://numpydoc.readthedocs.io/en/latest/format.html.

@mattip
Copy link
Member Author

mattip commented Nov 22, 2019

The docstrings are still not following the docstring standard

@charris Which ones?

@mattip
Copy link
Member Author

mattip commented Nov 22, 2019

@charris there is now an issue about improving documentation xref gh-14961.

@charris charris merged commit d76efa7 into numpy:master Nov 23, 2019
@charris
Copy link
Member

charris commented Nov 23, 2019

Thanks Matti.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants