Add tests for docstring Validation Script + py27 compat
State: closed

- [x] closes #xxxx
- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry

@jorisvandenbossche @toobaz a few edits here should make it work for both Py3 and Py2 users

Labels:
name: Testing
description: pandas testing functions or related to the test suite
name: Docs

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/20061?src=pr&el=h1) Report
> Merging [#20061](https://codecov.io/gh/pandas-dev/pandas/pull/20061?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/601d71fcfa9081fa88e5c20148972cd766396cd7?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/20061/graphs/tree.svg?width=650&height=150&src=pr&token=eZ4WkYLtcO)](https://codecov.io/gh/pandas-dev/pandas/pull/20061?src=pr&el=tree)

```diff
@@           Coverage Diff           @@
##           master   #20061   +/-   ##
=======================================
  Coverage   92.08%   92.08%           
=======================================
  Files         169      169           
  Lines       50706    50706           
=======================================
  Hits        46691    46691           
  Misses       4015     4015
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.49% <ø> (ø)` | :arrow_up: |
| #single | `42.33% <ø> (ø)` | :arrow_up: |


------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/20061?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/20061?src=pr&el=footer). Last update [601d71f...1fb5405](https://codecov.io/gh/pandas-dev/pandas/pull/20061?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

Tested, works great, thanks!
could you add a test for this somewhere? (need to make sure the test runner runs this); or can you have the lint.sh script actually run (a sub-set of this). that way we know changes to this will be good.
Fair point. The only issue with CI here is going to be the location of the files in the scripts folder, which I assume pytest nor the tests themselves would have access to given it's outside of the top level `pandas` package. `lint.sh` could fix that but I do think it would be better to have dedicated unit tests for the functionality.

Have to think it over but if you have any ideas on how to ideally configure the imports for testing let me know
can just add 

pandas/tests/scripts might be ok 
Hello @WillAyd! Thanks for updating the PR.





Cheers ! There are no PEP8 issues in this Pull Request. :beers: 

##### Comment last updated on August 13, 2018 at 20:40 Hours UTC
Latest commit will fail but should be a starting point for a test module. @jorisvandenbossche @datapythonista I took these examples directly from the guide, but it looks like even the examples in the guide fail the validation script. In this case do you know if it's the script or the examples that need to be updated?

Here are the major failures quasi-summarized:
- Parameters {'kwargs'} not documented
- Unknown parameters {'**kwargs'}
- Parameter "**kwargs" has not type
- No returns section found (some methods don't return anything)
- No see also section found (docs technically say these are optional)
- No examples section found (docs suggest these may be required, but aren't explicit)
- Parameters {'letters', 'length'} not documented (this is wrong for test `random_letters`)
- Docstring text should start in the line immediately... (`head` examples in docs are wrong)

Curious to hear how you would like to approach the above

With the new version of the script, an error of the kind:

```
Line 141, in pandas.DataFrame.apply
Failed example:
    df.apply(lambda x: Series([1, 2], index=['foo', 'bar']), axis=1)
Exception raised:
    Traceback (most recent call last):
      File "/usr/lib/python3.5/doctest.py", line 1321, in __run
        compileflags, 1), test.globs)
      File "<doctest pandas.DataFrame.apply[7]>", line 1, in <module>
        df.apply(lambda x: Series([1, 2], index=['foo', 'bar']), axis=1)
      File "/home/nobackup/repo/pandas/pandas/core/frame.py", line 5004, in apply
        return op.get_result()
      File "/home/nobackup/repo/pandas/pandas/core/apply.py", line 136, in get_result
        return self.apply_standard()
      File "/home/nobackup/repo/pandas/pandas/core/apply.py", line 242, in apply_standard
        self.apply_series_generator()
      File "/home/nobackup/repo/pandas/pandas/core/apply.py", line 271, in apply_series_generator
        results[i] = self.f(v)
      File "<doctest pandas.DataFrame.apply[7]>", line 1, in <lambda>
        df.apply(lambda x: Series([1, 2], index=['foo', 'bar']), axis=1)
    NameError: ("name 'Series' is not defined", 'occurred at index 0')
```

becomes under python2

```
Traceback (most recent call last):
  File "validate_docstrings.py", line 496, in <module>
    sys.exit(main(args.function))
  File "validate_docstrings.py", line 482, in main
    return validate_one(function)
  File "validate_docstrings.py", line 459, in validate_one
    examples_errs = doc.examples_errors
  File "validate_docstrings.py", line 265, in examples_errors
    runner.run(test, out=f.write)
  File "/usr/lib/python2.7/doctest.py", line 1454, in run
    return self.__run(test, compileflags, out)
  File "/usr/lib/python2.7/doctest.py", line 1363, in __run
    self.report_failure(out, test, example, got)
  File "/usr/lib/python2.7/doctest.py", line 1228, in report_failure
    self._checker.output_difference(example, got, self.optionflags))
TypeError: unicode argument expected, got 'str'
```

(obtained with ``python validate_docstrings.py pandas.DataFrame.apply``).
As I stated in gitter, fixing the remaining issues for py2 support would be great, but I think changing ``#!/usr/bin/env python`` to ``#!/usr/bin/env python3`` and telling users that they must have python 3 is also perfectly acceptable.
Thanks @toobaz and @jorisvandenbossche. So it looks like there are a couple of things to do here and with the sprint being tomorrow I'm wondering if it's even worth trying to get the compatibility to work and rather doing as @toobaz suggested and just explicitly requiring python3.

I think the bigger issue is that the test cases here taken from the doc don't pass the script. @jorisvandenbossche I saw your clarification on the `**kwargs` piece on the Google group, but curious what you think of the other issues? 
@datapythonista @jorisvandenbossche @TomAugspurger  in the last commit I blended a few things together, namely:

- Added the test cases
- Fixed Py27 support
- Fixed varargs and keyword args validation AND
- Relaxed errors on some sections (namely Extended Summary and Examples)

I copied most of the test cases from the document, but even then some of them weren't passing the validation script so I took a few liberties to modify. Ideally we would build out test cases to unit test particular aspects of the validation script, but for now I've just done a blanket pass for success / failures.
andas/tests/scripts/test_validate_docstrings.py ..............                                                                                                                                                                                                             [100%]
normal? maybe should catch this
```
================================================================================================================================ warnings summary =================================================================================================================================
pandas/tests/scripts/test_validate_docstrings.py::TestValidator::()::test_good_functions[sample_values]
  /Users/jreback/pandas/doc/sphinxext/numpydoc/docscrape.py:119: UserWarning: Unknown section Yields
    warn("Unknown section %s" % key)

-- Docs: http://doc.pytest.org/en/latest/warnings.html
```
The issue with 'Yields' stems from the fact that we include a copy of `numpydocstr` directly in pandas, but it doesn't appear to be a recent version (`Yields` support was added back in numpydoc 0.6)

Is there a particular reason we decided to copy that package into pandas rather than manage as a dependency? @jorisvandenbossche 
Long story involving many awful hacks :)

https://github.com/pandas-dev/pandas/issues/18147

I think the final blocker is https://github.com/numpy/numpydoc/pull/106

On Tue, Mar 13, 2018 at 11:27 AM, William Ayd <notifications@github.com>
wrote:

> The issue with 'Yields' stems from the fact that we include a copy of
> numpydocstr directly in pandas, but it doesn't appear to be a recent
> version (Yields support was added back in numpydoc 0.6)
>
> Is there a particular reason we decided to copy that package into pandas
> rather than manage as a dependency? @jorisvandenbossche
> <https://github.com/jorisvandenbossche>
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/20061#issuecomment-372728395>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHIh5OJJnGHIl2AEy0XJdw5IBzt5sBks5td_N3gaJpZM4SjWJQ>
> .
>

Woof...alright well I figure for now it's probably just easiest to port over 'Yields' support as it doesn't add that much overhead to what's already in place. Updated in latest push using https://github.com/numpy/numpydoc/pull/25/files as an implementation reference
> Woof...alright well I figure for now it's probably just easiest to port over 'Yields' support as it doesn't add that much overhead to what's already in place. Updated in latest push using https://github.com/numpy/numpydoc/pull/25/files as an implementation reference

Could you do this in a separate PR? 
I would like to keep the changes to our vendored version of numpydoc as self contained as possible to keep easier track of this. 
Actually we might also just try to completely update the vendored version with upstream (and fix the few things we need)
@jreback @jorisvandenbossche the latest push adds individual test cases to support specific components of the validation script. We have conflicting viewpoints about what to do with the changes I made to the `numpydoc` module to support adding `Yields`.

I'd be happy to try and add the tests for that module to the normal CI, but there are existing tests failing in there that I would need to fix that have nothing to do with this change - OK to do as a separate change? @jorisvandenbossche I also think it would be more trouble than it's worth to try and back out those changes from this change, as essentially it's bundled with the tests for the `Yield` section - OK to leave as is?
> @jorisvandenbossche I also think it would be more trouble than it's worth to try and back out those changes from this change, as essentially it's bundled with the tests for the Yield section - OK to leave as is?

I would like to keep it separate, but I can also do that if you want. 
@WillAyd FYI, I am currently working on a full update of numpydoc, so you can for now ignore my comment about the numpydoc changes.
@WillAyd PR is here: https://github.com/pandas-dev/pandas/pull/20383
@WillAyd this looks nice, especially the tests.  Is getting this over the finish line an active goal?
I figured this is less of a priority with the plans for Py27 but I'd be happy to rebase if you feel there's value in it
There are still lots of docstrings that need to be fixed, and this PR adds a lot of value to the validation script. May be not so important the py2 compatibility now that the sprint is over, but surely the tests will be very useful when fixing the script and adding new validations.
Just rebased though there's still one outstanding issue around the repr of some failures. Specifically, for something that looks like this in Python3:

```
'Parameters {'**kwargs'} not documented'
````

Is by default repr-ed as follows in Py2:

```
'Parameters set(['**kwargs']) not documented'
```

I think the two options are to:
 - Setting a different test expectation for Py2 vs Py3 OR
 - Muck around the code to unify the repr

I couldn't find a way to unify this out of the box so while I'd prefer the second option I don't want to clutter the code base too much for a rather trivial distinction. Curious @jbrockmendel and @datapythonista  what you think though
> Is by default repr-ed as follows in Py2:

```
from pandas.io.formats.printing import pprint_thing

s = {'**kwargs'}
out = 'Parameters {!s} not documented'.format(pprint_thing(s))

>>> print(out)
Parameters {**kwargs} not documented
```

Problem solved?
<b>Update</b> Shoot, wait, is the lack of quotations around kwargs a problem here?

```
>>> re.sub('^set\(\[(?P<g>.*)\]\)$', '{\g<g>}' , str(s))
"{'**kwargs'}"
```
@jbrockmendel thanks for the tip. I am under the impression that the quotes aren't that important, so I just updated the tests to assume the simple, compatible output
Travis failures appear unrelated. I think this is in a good spot but as always would appreciate reviews.

@jbrockmendel @datapythonista 
lgtm, but I don't think the tests are running in Travis, are they?
Where are you seeing that? Is there a collection output in the logs somewhere?
I checked the logs, and didn't see them, so I assumed they weren't executed. But you surely now more about travis than me.

May be you can push a failing test, and we'll know for sure. ;)