Skip to content

Fix minor error in dynamic load function #25256

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Feb 28, 2019
Merged

Fix minor error in dynamic load function #25256

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4b35573
Fix minor error in dynamic load function
nmusolino Feb 10, 2019
34152ef
Add basic tests of validate_docstrings.Docstring
nmusolino Feb 21, 2019
480a3de
Match message in validate_docstrings test
nmusolino Feb 22, 2019
64ec859
Use idiomatic pytest parameter unpacking
nmusolino Feb 28, 2019
7f7d577
Use standard pandas import in test_validate_docstrings
nmusolino Feb 28, 2019
f4704ea
Match AttributeError raised by getattr per review
nmusolino Feb 28, 2019
1b1ee48
Update test_validate_docstrings.py
WillAyd Feb 28, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions scripts/tests/test_validate_docstrings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
import textwrap
import pytest
import numpy as np
import pandas
Copy link
Member

Choose a reason for hiding this comment

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

import pandas as pd

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can change this.

I originally wrote import pandas so that the names and objects on lines 1011-1014 would correspond. But better to be consistent with other usage as you suggest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


import validate_docstrings
validate_one = validate_docstrings.validate_one

Expand Down Expand Up @@ -1004,6 +1006,30 @@ def test_item_subsection(self, idx, subsection):
assert result[idx][3] == subsection


class TestDocstringClass(object):
@pytest.mark.parametrize('name_and_expected_obj',
Copy link
Member

Choose a reason for hiding this comment

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

Minor stylistic change but you should just be able to do name, expected on this line and add name and expected as separate parameters to the function call instead of unpacking the tuple within your test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks! I didn't know that pytest would accept that syntax.

[('pandas.isnull', pandas.isnull),
('pandas.DataFrame', pandas.DataFrame),
('pandas.Series.sum', pandas.Series.sum)])
def test_resolves_class_name(self, name_and_expected_obj):
name, expected_obj = name_and_expected_obj
d = validate_docstrings.Docstring(name)
assert d.obj is expected_obj

@pytest.mark.parametrize('invalid_name', ['panda', 'panda.DataFrame'])
def test_raises_for_invalid_module_name(self, invalid_name):
# Note that the module names in this test are misspelled.
Copy link
Contributor

Choose a reason for hiding this comment

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

is this ImportError generated by us? or just a generic module name? maybe adding match= here?

Copy link
Contributor Author

@nmusolino nmusolino Feb 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, this is raised in the validate_docstrings.py script. That script takes a command-line argument like "pandas.DataFrame.head" or "pandas.isnull" (a string), then dynamically finds the function/method from the string. The script will then validate docstrings for that function only, which is useful for developers working on a particular docstring.

pandas/scripts/validate_docstrings.py

Lines 258 to 272 in 9c0f6a8

for maxsplit in range(1, name.count('.') + 1):
# TODO when py3 only replace by: module, *func_parts = ...
func_name_split = name.rsplit('.', maxsplit)
module = func_name_split[0]
func_parts = func_name_split[1:]
try:
obj = importlib.import_module(module)
except ImportError:
pass
else:
continue
if 'module' not in locals():
raise ImportError('No module can be imported '
'from "{}"'.format(name))

(Note that the snippet above is from master, and contains a bug in the boolean condition. This PR fixes the issue.)

I can make the test match the message like so... I don't want to over-specify the message here, so I could also just make sure that the invalid user-supplied is in the exception message.

    @pytest.mark.parametrize('invalid_name', ['panda', 'panda.DataFrame'])
    def test_raises_for_invalid_module_name(self, invalid_name):
        msg = 'No module can be imported.*{}'.format(invalid_name)
        with pytest.raises(ImportError, match=msg):
            validate_docstrings.Docstring(invalid_name)

with pytest.raises(ImportError):
validate_docstrings.Docstring(invalid_name)

@pytest.mark.parametrize('invalid_name',
['pandas.BadClassName',
'pandas.Series.bad_method_name'])
def test_raises_for_invalid_attribute_name(self, invalid_name):
with pytest.raises(AttributeError):
validate_docstrings.Docstring(invalid_name)


class TestMainFunction(object):
def test_exit_status_for_validate_one(self, monkeypatch):
monkeypatch.setattr(
Expand Down
2 changes: 1 addition & 1 deletion scripts/validate_docstrings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ def _load_obj(name):
else:
continue

if 'module' not in locals():
if 'obj' not in locals():
raise ImportError('No module can be imported '
'from "{}"'.format(name))

Expand Down