Integrate flake8_rst into ./ci/code_check.sh
State: closed

- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`

Integrated [flake8-rst](https://github.com/kataev/flake8-rst) into `code_check.sh` in order to check code in .. code-blocks:: python directives

* fixed all issues found with version v0.4.1
    - F821 undefined name ignored  -- too many occurences for now
    - W391 blank line at end of file  -- 4 occurences where I had no idea how to fix

```
doc/source/contributing_docstring.rst:629:9: W391 blank line at end of file
doc/source/contributing_docstring.rst:725:9: W391 blank line at end of file
doc/source/contributing_docstring.rst:815:9: W391 blank line at end of file
doc/source/contributing_docstring.rst:911:9: W391 blank line at end of file
```




Labels:
name: Docs
name: CI
description: Continuous Integration
name: Code Style
description: Code style, linting, code_checks

Comments:
Hello @FHaase! Thanks for submitting the PR.

 - There are no PEP8 issues in the file [`pandas/core/accessor.py`](https://github.com/FHaase/pandas/blob/bc6d6f7d34207f42ba14e248ca28fc819fcc8d12/pandas/core/accessor.py) !

 - There are no PEP8 issues in the file [`pandas/core/missing.py`](https://github.com/FHaase/pandas/blob/bc6d6f7d34207f42ba14e248ca28fc819fcc8d12/pandas/core/missing.py) !


# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/23381?src=pr&el=h1) Report
> Merging [#23381](https://codecov.io/gh/pandas-dev/pandas/pull/23381?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/f6917110611dcd1978286419529a85eca293fd00?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/23381/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/23381?src=pr&el=tree)

```diff
@@           Coverage Diff           @@
##           master   #23381   +/-   ##
=======================================
  Coverage   92.25%   92.25%           
=======================================
  Files         161      161           
  Lines       51262    51262           
=======================================
  Hits        47292    47292           
  Misses       3970     3970
```

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

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/23381?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/missing.py](https://codecov.io/gh/pandas-dev/pandas/pull/23381/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvbWlzc2luZy5weQ==) | `91.7% <ø> (ø)` | :arrow_up: |
| [pandas/core/accessor.py](https://codecov.io/gh/pandas-dev/pandas/pull/23381/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYWNjZXNzb3IucHk=) | `98.79% <ø> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/23381?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/23381?src=pr&el=footer). Last update [f691711...ed17b45](https://codecov.io/gh/pandas-dev/pandas/pull/23381?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

> F821 undefined name ignored -- too many occurences for now

If this gets merged, an issue should be created for this.
Really nice work, thank you for it.

But I don't think this is really validating the docstrings, just the rst files. We surely have plenty of pep8 errors in docstrings that are not detected here.

Besides that, in #23154, the goal was to validate pep8 of docstrings from `validate_docstrings.py`, so people working on docstrings can see of there is any pep8 issue at the same time of validating the rest of formats.

Happy to merge this, but let's remove the "Closes #23154" from the description, and let's create the issues for the pep8 errors being ignored as @gfyoung says.

Also, can't check well from my phone, but when adding flake8-rst to the dependencies, there are some files that are automatically generated. Did you edit them manually, or did you run `convert_deps.py`?


@jorisvandenbossche, @WillAyd, if you can also take a look here.
Okay let me summarize what I did:
* added flake8-rst to `./ci/environment-dev.yaml` then executed `./scripts/convert_deps.py
   - it changed requirement `tables` within `./ci/requirements-optional-pip.txt` to something with `pytables`.
        manually reverted back to `tables` as `pytables` does not seem to be within [pypi.org](https://pypi.org/search/?q=pytables)
* added flake8-rst commands to `./ci/code_checks.sh` 
   - flake8-rst pandas                       # searching within *.py files in pandas subfolder. 
   - flake8-rst doc --filename=*.rst   # searching within *.rst files in doc subfolder
* fixed all issues flake8-rst in version v0.4.1 finds executing both commands.
* added flake8-rst as dependency in `./ci/travis-36.yaml` to make travis pass.

What does flake8-rst do?
* looks for `.. code-blocks:: python` directives 
* cuts the code-block out and passes it into flake8
* reports the flake8 issues

Does it find everything? 
* No
   - `::` or `.. code-blocks::`                 # No specification of language python
   - `.. ipython:: python`                      # ipython currently not in RegEx 
   - `flake8-rst pandas --benchmark`  # reveals number of checked files is 3
* The intention of the plugin however is to find all these things.

Regarding integration in `validate_docstrings.py`: 
I didn't get it was specificly required to have it integrated there and thought having it in `./ci/code_checks.sh` would already be enough. **I'm going to take a look into that.**

Why do I think it closes #23154 ?
The PR introduces a mechanic that [if bugless] checks code in docstrings and rst files. Currently it isn't bugfree but that is not an issue with this project, but rather with flake8-rst itself and should be fixed over there.
@datapythonista 
calling `./ci/validate_docstrings.py` now calls and outputs results of:
* `flake8 --doctest <doc.source_file_name>?`
* `flake8-rst <doc.source_file_name>?`
`<doc.source_file_name>` only if a function name was passed.

`./ci/validate_docstrings.py pandas.read_excel` 
```
Flake8 reported issues:
Invoking command "flake8 --doctest pandas/util/_decorators.py" returned:
pandas/util/_decorators.py:320:26: F721 syntax error in doctest
pandas/util/_decorators.py:321:13: F721 syntax error in doctest
pandas/util/_decorators.py:322:15: F821 undefined name '_make_signature'
pandas/util/_decorators.py:322:31: F821 undefined name 'f'
```
Is that what you had in mind?
As I said, I was happy to merge the PR as it was. Just, we can't close the issue you referenced yet, as there is more to do.

If you can add those new changes in a separate PR I can give you more precise feedback. But summarizing, `validate_doctrings.py` is reporting a set of errors of different types, and PEP8 issues are just another kind of error. I don't like how flake8 is called as a new type of checked, and not as the rest of the checks. This makes things much more complicated, and doesn't consider the pep8 errors when the script is called without parameters to get the json report of all pending errors in the docstrings.

Besides that, I don't like using subprocess. I guess flake8-rst is a Python program, so we should be able to import it and call the appropriate functions.

In any case, validating the docstrings is for another PR, as it's independent of checking the code blocks.
![pep257](https://user-images.githubusercontent.com/41163037/47954362-e9279880-df89-11e8-9cf2-95789905dc25.png)
![pep8](https://user-images.githubusercontent.com/41163037/47954363-e9279880-df89-11e8-89d1-ec82b5714eba.png)

Found another one when looking for the occurence.
doesn't seem like the rendering is correct, is it? I guess what's expected is that the text shown is `PEP 8` and that is a link to the pep8 page. Can't check the links, but looks like there are two different links there?

Sorry if I'm misunderstanding.
`<a class="pep reference external" href="https://www.python.org/dev/peps/pep-0008#imports"><strong>PEP 8#imports</strong></a>`
That's the generated html. It looks a little strange because of the linebreak. The link works and points to the import section of the pep page.

I'd rather use the `:pep:` role than having a hardcoded link everywhere.

> **:pep:**
>    A reference to a Python Enhancement Proposal. This generates appropriate index entries. The text “PEP number” is generated; in the HTML output, this text is a hyperlink to an online copy of the specified PEP. You can link to a specific section by saying :pep:\`number#anchor\`.

thanks for the clarification, if that's the case all good.

@WillAyd can you take a look and merge if you're happy?
Before merging I'd like to change two more things:
* remove flake8_rst coverage from .py files as there won't be any `.. code-blocks::` within NumpyDocstrings. And flake8 with --docstrings options will cover it once #23399 is merged and integrated in ci.
* temporarily fix version to `0.4.2` as newer version will cover more types ( especially `.. ipython::`), thus suddenly letting travis fail.

I'm just waiting for any more suggestions to not trigger ci-builds. [skip ci] seems to not work with circle ci and azure pipeline.
I wouldn't worry that much about the CI, and I'd leave the PR in a state that you're happy and it can be merged.
Okay can merge if wanted
@WillAyd can you take another look, and merge on green if you're happy.

Thanks @FHaase for the work on this, really nice PR. If you want to create two issues for the errors you're ignoring, that would be nice. I'm sure you'll be better than me at explaining what are the problems. You can mention this PR, and mention me too if you want.
@datapythonista note this will slightly conflict with your revision of ci deps (need to add the new package)
no problem, I can update the other two PRs, their CI is still waiting, so it's a good time
@FHaase can you fix the conflicts please? Sorry this wasn't merged before.
@datapythonista No Problem. Currently fixing issues within the docs getting covered by an updated flake8-rst. Is it possible to add a PR that builds on an unmerged PR?
It's possible, but makes things a bit tricky, so I'd better wait.

@jreback @WillAyd can you take a look at this again? Would be nice to get it merged once it's green.
Thanks @FHaase, nice work. It's great to have all those issues fixed, and validation in the rst files.