-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
Changes to validate_docstring script to be able to check all docstrings at once #22408
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
Changes from 12 commits
b31fc10
5d2815f
5bb7b9b
86327d4
188f4a3
0d25298
577308e
156b997
3a5e7fd
1a8aee9
aa4807a
1d2c5c0
db782fd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
import string | ||
import random | ||
import io | ||
import pytest | ||
import numpy as np | ||
|
||
|
@@ -531,28 +532,36 @@ def _import_path(self, klass=None, func=None): | |
|
||
@capture_stderr | ||
def test_good_class(self): | ||
assert validate_one(self._import_path( | ||
klass='GoodDocStrings')) == 0 | ||
errors = validate_one(self._import_path( | ||
klass='GoodDocStrings'))['errors'] | ||
assert isinstance(errors, list) | ||
assert not errors | ||
|
||
@capture_stderr | ||
@pytest.mark.parametrize("func", [ | ||
'plot', 'sample', 'random_letters', 'sample_values', 'head', 'head1', | ||
'contains', 'mode']) | ||
def test_good_functions(self, func): | ||
assert validate_one(self._import_path( | ||
klass='GoodDocStrings', func=func)) == 0 | ||
errors = validate_one(self._import_path( | ||
klass='GoodDocStrings', func=func))['errors'] | ||
assert isinstance(errors, list) | ||
assert not errors | ||
|
||
@capture_stderr | ||
def test_bad_class(self): | ||
assert validate_one(self._import_path( | ||
klass='BadGenericDocStrings')) > 0 | ||
errors = validate_one(self._import_path( | ||
klass='BadGenericDocStrings'))['errors'] | ||
assert isinstance(errors, list) | ||
assert errors | ||
|
||
@capture_stderr | ||
@pytest.mark.parametrize("func", [ | ||
'func', 'astype', 'astype1', 'astype2', 'astype3', 'plot', 'method']) | ||
def test_bad_generic_functions(self, func): | ||
assert validate_one(self._import_path( # noqa:F821 | ||
klass='BadGenericDocStrings', func=func)) > 0 | ||
errors = validate_one(self._import_path( # noqa:F821 | ||
klass='BadGenericDocStrings', func=func))['errors'] | ||
assert isinstance(errors, list) | ||
assert errors | ||
|
||
@pytest.mark.parametrize("klass,func,msgs", [ | ||
# Summary tests | ||
|
@@ -594,7 +603,82 @@ def test_bad_generic_functions(self, func): | |
marks=pytest.mark.xfail) | ||
]) | ||
def test_bad_examples(self, capsys, klass, func, msgs): | ||
validate_one(self._import_path(klass=klass, func=func)) # noqa:F821 | ||
err = capsys.readouterr().err | ||
result = validate_one(self._import_path(klass=klass, func=func)) # noqa:F821 | ||
for msg in msgs: | ||
assert msg in err | ||
assert msg in ' '.join(result['errors']) | ||
|
||
|
||
class TestApiItems(object): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also minor but since this test isn't used for any testing (i.e. not collected by the runner) can we remove the |
||
@property | ||
def api_doc(self): | ||
return io.StringIO(''' | ||
.. currentmodule:: itertools | ||
|
||
Itertools | ||
--------- | ||
|
||
Infinite | ||
~~~~~~~~ | ||
|
||
.. autosummary:: | ||
|
||
cycle | ||
count | ||
|
||
Finite | ||
~~~~~~ | ||
|
||
.. autosummary:: | ||
|
||
chain | ||
|
||
.. currentmodule:: random | ||
|
||
Random | ||
------ | ||
|
||
All | ||
~~~ | ||
|
||
.. autosummary:: | ||
|
||
seed | ||
randint | ||
''') | ||
|
||
@pytest.mark.parametrize('idx,name', [(0, 'itertools.cycle'), | ||
(1, 'itertools.count'), | ||
(2, 'itertools.chain'), | ||
(3, 'random.seed'), | ||
(4, 'random.randint')]) | ||
def test_item_name(self, idx, name): | ||
res = list(validate_docstrings.get_api_items(self.api_doc)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you use |
||
assert res[idx][0] == name | ||
|
||
@pytest.mark.parametrize('idx,func', [(0, 'cycle'), | ||
(1, 'count'), | ||
(2, 'chain'), | ||
(3, 'seed'), | ||
(4, 'randint')]) | ||
def test_item_function(self, idx, func): | ||
res = list(validate_docstrings.get_api_items(self.api_doc)) | ||
assert callable(res[idx][1]) | ||
assert res[idx][1].__name__ == func | ||
|
||
@pytest.mark.parametrize('idx,section', [(0, 'Itertools'), | ||
(1, 'Itertools'), | ||
(2, 'Itertools'), | ||
(3, 'Random'), | ||
(4, 'Random')]) | ||
def test_item_section(self, idx, section): | ||
res = list(validate_docstrings.get_api_items(self.api_doc)) | ||
assert res[idx][2] == section | ||
|
||
@pytest.mark.parametrize('idx,subsection', [(0, 'Infinite'), | ||
(1, 'Infinite'), | ||
(2, 'Finite'), | ||
(3, 'All'), | ||
(4, 'All')]) | ||
def test_item_subsection(self, idx, subsection): | ||
res = list(validate_docstrings.get_api_items(self.api_doc)) | ||
assert res[idx][3] == subsection |
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.
Minor nit but what does this add? Can't we just do the normal inclusion check against the list instead of joining into a string?
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.
msg in result['errors']
is different than this, for example'a' in 'foo bar'
isTrue
, buta in ['foo', 'bar']
isFalse
.I think the
.join()
is simpler than another loop, is it what you would do, or you were thinking on the previous case?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.
No but I thought
msg
would explicitly match one of theerrors
; will take a look more deeply on next review if that assumption is incorrectThere 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.
Debugging this locally still don't think the string concatenation is necessary?