DOC: Validate that See Also section items do not contain the pandas. prefix #23145

thoo · 2018-10-14T08:11:23Z

Fix #23136
Please also see similar PR #23143

codecov · 2018-10-14T08:38:14Z

Codecov Report

Merging #23145 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #23145   +/-   ##
=======================================
  Coverage   92.22%   92.22%           
=======================================
  Files         169      169           
  Lines       51261    51261           
=======================================
  Hits        47277    47277           
  Misses       3984     3984

Flag	Coverage Δ
#multiple	`90.66% <ø> (ø)`	⬆️
#single	`42.23% <ø> (ø)`	⬆️

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 f662c5f...0461c99. Read the comment docs.

getschomp · 2018-10-14T13:36:13Z

The one I posted is a slightly different issue also affecting the 'See Also' section.(23135/23136) I think you may have missed pushing a commit somehow, because I'm only seeing the blank line changes.

pep8speaks · 2018-10-14T14:15:50Z

Hello @thoo! Thanks for updating the PR.

There are no PEP8 issues in the file scripts/tests/test_validate_docstrings.py !
There are no PEP8 issues in the file scripts/validate_docstrings.py !

Comment last updated on October 15, 2018 at 17:29 Hours UTC

thoo · 2018-10-14T14:17:47Z

@getschomp Thanks. I just updated.

datapythonista

Good start, added some comments, still some things to be fixed.

doc/source/cookbook.rst

datapythonista · 2018-10-14T15:13:22Z

scripts/validate_docstrings.py

@@ -499,7 +499,8 @@ def validate_one(func_name):
            if not rel_desc:
                errs.append('Missing description for '
                            'See Also "{}" reference'.format(rel_name))
-
+            if rel_name[:7].lower() == 'pandas.':


I think it's more pythonic to write if rel_name.startwith('pandas.'): (or you could do the .lower() before the startswith() but I don't think it's needed here.

datapythonista · 2018-10-14T15:16:28Z

scripts/validate_docstrings.py

@@ -499,7 +499,8 @@ def validate_one(func_name):
            if not rel_desc:
                errs.append('Missing description for '
                            'See Also "{}" reference'.format(rel_name))
-
+            if rel_name[:7].lower() == 'pandas.':
+                errs.append('{} should not have prefix `pandas`'.format(rel_name))


I think the error message is good, but it'd be even more explicit to say something like 'pandas.DataFrame.head' in the 'See Also' section does not need the 'pandas' prefix, use 'DataFrame.head' instead. What do you think?

The PEP8 validation reported a line > 80 characters, I guess is this one.

datapythonista · 2018-10-14T15:19:15Z

scripts/tests/test_validate_docstrings.py

@@ -334,6 +334,14 @@ def method(self, foo=None, bar=None):
        pass


+class BadSeeAlso(object):


Can you place the test classes in the same order as the sections in the docstring (summaries, parameters, returns, see also...)

datapythonista · 2018-10-14T15:21:04Z

scripts/tests/test_validate_docstrings.py

+    def prefix_pandas(self):
+        """
+        Return  prefix with `pandas` from See Also sec
+        """


What's bad in this test? It doesn't have the See Also section, so the new error will not be raised here.

@datapythonista Can I get more hints ? :). I wasn't sure what I was doing in the test section. Thanks.

it's difficult to explain unit testing in a github comment, but the idea of a unit test is that you want to check that things work automatically. I guess after implementing the error for the pandas prefix, you run the program for a docstring with the prefix, and saw that the error was correctly reported. You need to do the same here, but with code. Most of the stuff is already build, the main thing to do is to write a docstring that should generate the error (this is what goes in this part of the code). And then a program runs the validate_docstrings.py, and makes sure that the error is generated. This is already implemented if you add the method with the docstring you want to validate, and the expected message in the @pytest.mark.parametrize you updated.

datapythonista · 2018-10-14T15:21:33Z

scripts/tests/test_validate_docstrings.py

@@ -564,6 +572,9 @@ def test_bad_generic_functions(self, func):
        assert errors

    @pytest.mark.parametrize("klass,func,msgs", [
+        #SeeAlso tests


There are missing spaces here, including after the # which is a formatting error.

thoo · 2018-10-14T15:24:40Z

Thanks. I will work on them.

thoo

@datapythonista Could you please take a look? Hope everything is resolved.

datapythonista

Looking better, added some comments.

datapythonista · 2018-10-15T15:21:18Z

doc/source/cookbook.rst

@@ -1234,25 +1234,19 @@ The `method` argument within `DataFrame.corr` can accept a callable in addition
        n = len(x)
        a = np.zeros(shape=(n, n))
        b = np.zeros(shape=(n, n))
-


can you make sure this file does not appear in the Files changes diff. The cookbook should be untouched in this PR.

datapythonista · 2018-10-15T15:22:46Z

scripts/tests/test_validate_docstrings.py

+
+        See Also
+        --------
+        pandas.Series.rename : Alter Series index labels or name


Can you add a period at he end? This is required in the see also description, and this test should only have wrong the pandas. prefix.

Can you add another item, but without the pandas prefix, so we see that when there is more than one item the reported one is the wrong one.

datapythonista · 2018-10-15T15:24:28Z

scripts/tests/test_validate_docstrings.py

+
+    def prefix_pandas(self):
+        """
+        Return  prefix with `pandas` from See Also sec


Can you briefly describe the test here. It'll be more useful than this sentence (see other tests for reference). Also, make sure there are no two spaces together, and that the text finishes with a period.

datapythonista · 2018-10-15T15:25:22Z

scripts/tests/test_validate_docstrings.py

@@ -600,7 +613,10 @@ def test_bad_generic_functions(self, func):
        pytest.param('BadReturns', 'no_description', ('foo',),
                     marks=pytest.mark.xfail),
        pytest.param('BadReturns', 'no_punctuation', ('foo',),
-                     marks=pytest.mark.xfail)
+                     marks=pytest.mark.xfail),
+        # SeeAlso tests


missing space between See Also

datapythonista · 2018-10-15T15:27:21Z

scripts/tests/test_validate_docstrings.py

+                     marks=pytest.mark.xfail),
+        # SeeAlso tests
+        ('BadSeeAlso', 'prefix_pandas',
+         ('section does not need `pandas` prefix',)),


can you check for the whole message. It's useful to see what's the exact error reported, and that the item being reported is indeed the one with the prefix.

datapythonista

not sure if one of the lines is more than 80 characters, but if the CI is green, lgtm.

@WillAyd do you want to take a look?

datapythonista · 2018-10-24T15:51:40Z

scripts/validate_docstrings.py

+            if rel_name.startswith('pandas.'):
+                errs.append('{} in `See Also` section does not '
+                            'need `pandas` prefix, use {} instead.'
+                            .format(rel_name, rel_name.replace('pandas.', '')))


I'd use rel_name[len('pandas.'):] instead of the replace. I don't think in practice will make a difference, but in a case like pandas.somemod.pandas.MyClass the replace wouldn't work as expected.

Sure. I will update it right away.

datapythonista

lgtm, thanks!

datapythonista · 2018-10-25T05:43:03Z

Can you check the azure error, and make sure it's green, so we can merge?

thoo · 2018-10-26T05:05:08Z

Can you check the azure error, and make sure it's green, so we can merge?

The error is from miniconda package.

2018-10-24T17:02:31.2718145Z C:\Miniconda\envs\pandas\lib\site-packages\matplotlib\colorbar.py:28: in <module>
2018-10-24T17:02:31.2718445Z     import matplotlib.artist as martist
2018-10-24T17:02:31.2719232Z E   AttributeError: module 'matplotlib' has no attribute 'artist'
2018-10-24T17:02:31.4445103Z ##[error]Cmd.exe exited with code '4'.

…ndas * repo_org/master: (23 commits) DOC: Add docstring validations for "See Also" section (pandas-dev#23143) TST: Fix test assertion (pandas-dev#23357) BUG: Handle Period in combine (pandas-dev#23350) REF: SparseArray imports (pandas-dev#23329) CI: Migrate some CircleCI jobs to Azure (pandas-dev#22992) DOC: update the is_month_start/is_month_end docstring (pandas-dev#23051) Partialy fix issue pandas-dev#23334 - isort pandas/core/groupby directory (pandas-dev#23341) TST: Add base test for extensionarray setitem pandas-dev#23300 (pandas-dev#23304) API: Add sparse Acessor (pandas-dev#23183) PERF: speed up CategoricalIndex.get_loc (pandas-dev#23235) fix and test incorrect case in delta_to_nanoseconds (pandas-dev#23302) BUG: Handle Datetimelike data in DataFrame.combine (pandas-dev#23317) TST: re-enable gbq tests (pandas-dev#23303) Switched references of App veyor to azure pipelines in the contributing CI section (pandas-dev#23311) isort imports-io (pandas-dev#23332) DOC: Added a Multi Index example for the Series.sum method (pandas-dev#23279) REF: Make PeriodArray an ExtensionArray (pandas-dev#22862) DOC: Added Examples for Series max (pandas-dev#23298) API/ENH: tz_localize handling of nonexistent times: rename keyword + add shift option (pandas-dev#22644) BUG: Let MultiIndex.set_levels accept any iterable (pandas-dev#23273) (pandas-dev#23291) ...

datapythonista · 2018-10-27T09:05:56Z

@WillAyd, can you take a look and merge if you're happy?

WillAyd · 2018-10-27T18:20:22Z

Thanks @thoo

…xamples * repo_org/master: DOC: Validate that See Also section items do not contain the pandas. prefix (pandas-dev#23145) API/TST: make hasnans always return python booleans (pandas-dev#23349)

…prefix (pandas-dev#23145)

datapythonista changed the title ~~Warning on Prefix pandas~~ DOC: Validate that See Also section items do not contain the pandas. prefix Oct 14, 2018

datapythonista added Docs CI Continuous Integration labels Oct 14, 2018

datapythonista requested changes Oct 14, 2018

View reviewed changes

thoo commented Oct 15, 2018

View reviewed changes

datapythonista reviewed Oct 15, 2018

View reviewed changes

thoo force-pushed the Warning_prefix_pandas branch from bcb33b8 to 6e7ab2a Compare October 15, 2018 15:46

datapythonista approved these changes Oct 15, 2018

View reviewed changes

datapythonista reviewed Oct 24, 2018

View reviewed changes

thoo added 7 commits October 24, 2018 12:46

Give Warning on prefix pandas

f85e296

Update error message and test validator

6a64a62

Fix string prasing and test validator

9358fae

PEP8 compliant fixes

4cf7e1b

Fix in prefix_pandas and mark.parameterize

785a7c0

Fix long string and remove a misplaced comma

0209430

Made changes as recommended

67ed7f4

thoo force-pushed the Warning_prefix_pandas branch from 6a0d30a to 67ed7f4 Compare October 24, 2018 16:47

datapythonista approved these changes Oct 25, 2018

View reviewed changes

thoo added 2 commits October 26, 2018 23:47

modify due to merge with upstream/master

0461c99

WillAyd approved these changes Oct 27, 2018

View reviewed changes

WillAyd merged commit da9d851 into pandas-dev:master Oct 27, 2018

tm9k1 pushed a commit to tm9k1/pandas that referenced this pull request Nov 19, 2018

DOC: Validate that See Also section items do not contain the pandas. …

afe211c

…prefix (pandas-dev#23145)

thoo deleted the Warning_prefix_pandas branch January 2, 2019 20:26

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

DOC: Validate that See Also section items do not contain the pandas. …

ebba3af

…prefix (pandas-dev#23145)

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

DOC: Validate that See Also section items do not contain the pandas. …

9cfede6

…prefix (pandas-dev#23145)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DOC: Validate that See Also section items do not contain the pandas. prefix #23145

DOC: Validate that See Also section items do not contain the pandas. prefix #23145

thoo commented Oct 14, 2018

codecov bot commented Oct 14, 2018 •

edited

Loading

getschomp commented Oct 14, 2018

pep8speaks commented Oct 14, 2018 •

edited

Loading

thoo commented Oct 14, 2018

datapythonista left a comment

datapythonista Oct 14, 2018

datapythonista Oct 14, 2018

datapythonista Oct 14, 2018

datapythonista Oct 14, 2018

thoo Oct 14, 2018

datapythonista Oct 14, 2018

datapythonista Oct 14, 2018

thoo commented Oct 14, 2018

thoo left a comment

datapythonista left a comment

datapythonista Oct 15, 2018

datapythonista Oct 15, 2018

datapythonista Oct 15, 2018

datapythonista Oct 15, 2018

datapythonista Oct 15, 2018

datapythonista left a comment

datapythonista Oct 24, 2018 •

edited

Loading

thoo Oct 24, 2018

datapythonista left a comment

datapythonista commented Oct 25, 2018 •

edited

Loading

thoo commented Oct 26, 2018

datapythonista commented Oct 27, 2018

WillAyd commented Oct 27, 2018

		@@ -334,6 +334,14 @@ def method(self, foo=None, bar=None):
		pass


		class BadSeeAlso(object):

DOC: Validate that See Also section items do not contain the pandas. prefix #23145

DOC: Validate that See Also section items do not contain the pandas. prefix #23145

Conversation

thoo commented Oct 14, 2018

codecov bot commented Oct 14, 2018 • edited Loading

Codecov Report

getschomp commented Oct 14, 2018

pep8speaks commented Oct 14, 2018 • edited Loading

Comment last updated on October 15, 2018 at 17:29 Hours UTC

thoo commented Oct 14, 2018

datapythonista left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thoo commented Oct 14, 2018

thoo left a comment

Choose a reason for hiding this comment

datapythonista left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

datapythonista left a comment

Choose a reason for hiding this comment

datapythonista Oct 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

datapythonista left a comment

Choose a reason for hiding this comment

datapythonista commented Oct 25, 2018 • edited Loading

thoo commented Oct 26, 2018

datapythonista commented Oct 27, 2018

WillAyd commented Oct 27, 2018

codecov bot commented Oct 14, 2018 •

edited

Loading

pep8speaks commented Oct 14, 2018 •

edited

Loading

datapythonista Oct 24, 2018 •

edited

Loading

datapythonista commented Oct 25, 2018 •

edited

Loading