Integrate flake8_rst into ./ci/code_check.sh #23381

FHaase · 2018-10-27T11:16:45Z

tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff

Integrated 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

Added flake8-rst as development dependency Added flake8-rst configuration Added checks to code_checks.sh Signed-off-by: Fabian Haase <[email protected]>

Signed-off-by: Fabian Haase <[email protected]>

cookbook.rst:971 -> python of .. code-block:: deleted to disable flake8-rst checks. %timeit is invalid Syntax, # noqa: E999 does not have an effect. Signed-off-by: Fabian Haase <[email protected]>

Signed-off-by: Fabian Haase <[email protected]>

readding F401 detection Signed-off-by: Fabian Haase <[email protected]>

# Conflicts: # ci/code_checks.sh

pep8speaks · 2018-10-27T11:16:48Z

Hello @FHaase! Thanks for submitting the PR.

There are no PEP8 issues in the file pandas/core/accessor.py !
There are no PEP8 issues in the file pandas/core/missing.py !

adding flake8-rst to travis-36.yaml reverting changes to requirements-optional-pip.txt pytables seems to be tables on PyPI [running ./scripts/convert_deps.py previously changed it] Signed-off-by: Fabian Haase <[email protected]>

codecov · 2018-10-27T15:09:32Z

Codecov Report

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

@@           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% <ø> (ø)`	⬆️
#single	`42.33% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
pandas/core/missing.py	`91.7% <ø> (ø)`	⬆️
pandas/core/accessor.py	`98.79% <ø> (ø)`	⬆️

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 f691711...ed17b45. Read the comment docs.

gfyoung · 2018-10-27T22:07:13Z

F821 undefined name ignored -- too many occurences for now

If this gets merged, an issue should be created for this.

datapythonista · 2018-10-28T01:12:37Z

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.

FHaase · 2018-10-28T09:59:30Z

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
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.

FHaase · 2018-10-28T12:17:52Z

@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?

datapythonista · 2018-10-28T12:58:52Z

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.

WillAyd · 2018-10-30T22:59:42Z

doc/source/basics.rst

@@ -302,23 +302,17 @@ To evaluate single-element pandas objects in a boolean context, use the method

 .. warning::

-   You might be tempted to do the following:
+   Using a DataFrame as a condition will raise errors,


What was the point of changing this? We always ask changes to have one focus to keep diff minimal. Would prefer if you could back out anything unrelated to the flake8 and do in a separate PR

Signed-off-by: Fabian Haase <[email protected]>

WillAyd · 2018-10-31T19:50:56Z

doc/source/basics.rst


   These will both raise errors, as you are trying to compare multiple values.

-   .. code-block:: python
+   .. code-block:: console


Does python-traceback render here? Might be a better choice than console

👍 didn't know that directive.

WillAyd · 2018-10-31T19:51:56Z

doc/source/basics.rst


   Or

   .. code-block:: python

-       >>> df and df2
+       >>> df and df2                     # noqa: E999


Generally what kind of E999 errors are you getting? Not terribly familiar with this code but seems to come as a result of AST issues, but am wondering if that's more of a Python versioning issue than an actual styling violation

I guess the and tricked me into thinking it was an if-statement like a couple of lines above.

WillAyd · 2018-10-31T19:54:25Z

doc/source/contributing_docstring.rst

@@ -189,31 +189,34 @@ infinitive verb.

 .. code-block:: python

-    def astype(dtype):
+    def astype(dtype):    # noqa: F811


Is the noqa needed for the first definition? Would ideally like to minimize if not eliminate the commented exceptions as I could imagine a scenario where it is confusing to first time contributors

At that position no, I went for symmetry. Indeed using noqa is quite confusing and therefore I try to avoid it.
@WillAyd At this position I thought putting every method in an extra .. code-block:: would be the best option, however it changes the documentation quite signifikant?!

WillAyd · 2018-10-31T19:56:06Z

doc/source/contributing_docstring.rst


 Any other module used in the examples must be explicitly imported, one per line (as
-recommended in `PEP-8 <https://www.python.org/dev/peps/pep-0008/#imports>`_)
+recommended in :pep:`8#imports`)


Does this directive always work or does it require an external plugin?

its integrated directly in sphinx

FHaase · 2018-10-31T22:57:27Z

doc/source/contributing.rst

@@ -744,15 +744,15 @@ Transitioning to ``pytest``
 .. code-block:: python

    class TestReallyCoolFeature(object):
-        ....
+        ....                                  # noqa: E999


@WillAyd Would you prefer a pass instead of the noqa?

Signed-off-by: Fabian Haase <[email protected]>

datapythonista

Really nice work. I'd possibly use pass instead of the ... everywhere, but just a preference.

Looking forward to have all these fixes merged, and to start validating pep8 in docstrings.

Btw, if you have rendered the output of the pep directive and you can add a screenshot of it, would be nice, I don't know how that is being rendered.

FHaase · 2018-11-03T16:03:14Z

Found another one when looking for the occurence.

datapythonista · 2018-11-03T16:16:01Z

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.

FHaase · 2018-11-03T20:14:32Z

<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`.

datapythonista · 2018-11-04T03:50:36Z

thanks for the clarification, if that's the case all good.

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

FHaase · 2018-11-04T14:12:31Z

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 ENH: Use flake8 to check for PEP8 violations in doctests #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.

datapythonista · 2018-11-04T15:50:44Z

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.

FHaase · 2018-11-04T16:49:50Z

Okay can merge if wanted

datapythonista · 2018-11-04T17:03:41Z

@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.

Signed-off-by: Fabian Haase <[email protected]>

jreback · 2018-11-06T14:01:03Z

@datapythonista note this will slightly conflict with your revision of ci deps (need to add the new package)

datapythonista · 2018-11-06T14:04:54Z

no problem, I can update the other two PRs, their CI is still waiting, so it's a good time

datapythonista · 2018-11-09T12:30:29Z

@FHaase can you fix the conflicts please? Sorry this wasn't merged before.

…ake8-rst

FHaase · 2018-11-09T14:38:49Z

@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?

datapythonista · 2018-11-09T14:43:01Z

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.

jorisvandenbossche

Looks good to me!

datapythonista · 2018-11-09T16:27:05Z

Thanks @FHaase, nice work. It's great to have all those issues fixed, and validation in the rst files.

FHaase added 8 commits October 25, 2018 13:05

Add flake8-rst

8325caf

Added flake8-rst as development dependency Added flake8-rst configuration Added checks to code_checks.sh Signed-off-by: Fabian Haase <[email protected]>

Fixed PEP8 issues in docstrings

226bc53

Signed-off-by: Fabian Haase <[email protected]>

Fixing PEP8 issues within code-blocks

472df59

Fixing PEP8 issues

11a3632

Signed-off-by: Fabian Haase <[email protected]>

Fixing PEP8 issues

7fd215d

cookbook.rst:971 -> python of .. code-block:: deleted to disable flake8-rst checks. %timeit is invalid Syntax, # noqa: E999 does not have an effect. Signed-off-by: Fabian Haase <[email protected]>

Fixing PEP8 issues

d1be499

Signed-off-by: Fabian Haase <[email protected]>

Fixing PEP8 issues

acea30a

readding F401 detection Signed-off-by: Fabian Haase <[email protected]>

Merge branch 'master' into feature/flake8-rst

bc6d6f7

# Conflicts: # ci/code_checks.sh

Fix travis build

77df97f

adding flake8-rst to travis-36.yaml reverting changes to requirements-optional-pip.txt pytables seems to be tables on PyPI [running ./scripts/convert_deps.py previously changed it] Signed-off-by: Fabian Haase <[email protected]>

FHaase force-pushed the feature/flake8-rst branch from 421577e to 77df97f Compare October 27, 2018 14:11

jschendel added Docs CI Continuous Integration Code Style Code style, linting, code_checks labels Oct 27, 2018

gfyoung requested review from datapythonista and jreback and removed request for datapythonista October 27, 2018 22:07

FHaase force-pushed the feature/flake8-rst branch from 3624260 to ee36f34 Compare October 28, 2018 12:07

FHaase force-pushed the feature/flake8-rst branch from 31fb4dd to 77df97f Compare October 28, 2018 14:25

FHaase mentioned this pull request Oct 29, 2018

ENH: Use flake8 to check for PEP8 violations in doctests #23399

Merged

2 tasks

WillAyd requested changes Oct 30, 2018

View reviewed changes

FHaase added 2 commits October 31, 2018 18:58

revert back to minimal changes in doc folder

67cf043

Signed-off-by: Fabian Haase <[email protected]>

Merge branch 'master' into feature/flake8-rst

f440c63

WillAyd requested changes Oct 31, 2018

View reviewed changes

FHaase commented Oct 31, 2018

View reviewed changes

FHaase added 2 commits November 1, 2018 11:25

Cleanup

948f176

Signed-off-by: Fabian Haase <[email protected]>

Optional noqa removes

c01e22d

Signed-off-by: Fabian Haase <[email protected]>

datapythonista approved these changes Nov 3, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into feature/flake8-rst

816f26e

Signed-off-by: Fabian Haase <[email protected]>

FHaase force-pushed the feature/flake8-rst branch from 403cc95 to 816f26e Compare November 4, 2018 20:07

FHaase mentioned this pull request Nov 5, 2018

Discussion: General ignore F821 within documentation #23508

Closed

FHaase added 2 commits November 9, 2018 15:11

Merge remote-tracking branch 'upstream/master' into feature/flake8-rst

1a4c6fe

Merge branch 'master' of github.com:pandas-dev/pandas into feature/fl…

ed17b45

…ake8-rst

jorisvandenbossche approved these changes Nov 9, 2018

View reviewed changes

datapythonista merged commit efd1844 into pandas-dev:master Nov 9, 2018

JustinZhengBC pushed a commit to JustinZhengBC/pandas that referenced this pull request Nov 14, 2018

DOC/CI: Add linting to rst files, and fix issues (pandas-dev#23381)

56b8024

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

DOC/CI: Add linting to rst files, and fix issues (pandas-dev#23381)

373301b

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

DOC/CI: Add linting to rst files, and fix issues (pandas-dev#23381)

aed3852

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

DOC/CI: Add linting to rst files, and fix issues (pandas-dev#23381)

e458318

Uh oh!

Integrate flake8_rst into ./ci/code_check.sh #23381

Integrate flake8_rst into ./ci/code_check.sh #23381

Uh oh!

Conversation

FHaase commented Oct 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pep8speaks commented Oct 27, 2018

Uh oh!

codecov bot commented Oct 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

gfyoung commented Oct 27, 2018

Uh oh!

datapythonista commented Oct 28, 2018

Uh oh!

FHaase commented Oct 28, 2018

Uh oh!

FHaase commented Oct 28, 2018

Uh oh!

datapythonista commented Oct 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FHaase Oct 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

datapythonista left a comment

Choose a reason for hiding this comment

Uh oh!

FHaase commented Nov 3, 2018

Uh oh!

datapythonista commented Nov 3, 2018

Uh oh!

FHaase commented Nov 3, 2018

Uh oh!

datapythonista commented Nov 4, 2018

Uh oh!

FHaase commented Nov 4, 2018

Uh oh!

datapythonista commented Nov 4, 2018

Uh oh!

FHaase commented Nov 4, 2018

Uh oh!

datapythonista commented Nov 4, 2018

Uh oh!

jreback commented Nov 6, 2018

Uh oh!

datapythonista commented Nov 6, 2018

Uh oh!

datapythonista commented Nov 9, 2018

Uh oh!

FHaase commented Nov 9, 2018

Uh oh!

datapythonista commented Nov 9, 2018

Uh oh!

jorisvandenbossche left a comment

Choose a reason for hiding this comment

Uh oh!

datapythonista commented Nov 9, 2018

Uh oh!

FHaase commented Oct 27, 2018 •

edited

Loading

codecov bot commented Oct 27, 2018 •

edited

Loading

FHaase Oct 31, 2018 •

edited

Loading