DOC: Upgrade flake8-rst version #23847

FHaase · 2018-11-21T21:45:34Z

Update of flake8-rst version to >=0.6.0
Add files with issues to exclude

pep8speaks · 2018-11-21T21:45:36Z

Hello @FHaase! Thanks for submitting the PR.

There are no PEP8 issues in the file doc/source/conf.py !

datapythonista · 2018-11-22T01:05:20Z

There are several issues like #23790, #23791 and ##23794 that people is working on, and will conflict with this PR I think. If you can try to coordinate, so we avoid duplicating efforts...

Also, unless you expect the new flake8-rst release to be immediate, I'd ignore F821, so we can merge this fast, and we avoid conflicts.

Ping me when you get it sorted, and I'll review the changes.

FHaase · 2018-11-22T12:18:40Z

@datapythonista I've reverted changes in advanced.rst and cookbook.rst. io.rst hasn't been touched but excluded from checks.

The PR's got merged a couple of hours ago so 0.6.0 is finally out. It has taken me some time but I think it's now usable and I'm going to open issues for the excluded files.

codecov · 2018-11-22T12:42:57Z

Codecov Report

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

@@           Coverage Diff           @@
##           master   #23847   +/-   ##
=======================================
  Coverage   42.46%   42.46%           
=======================================
  Files         161      161           
  Lines       51557    51557           
=======================================
  Hits        21892    21892           
  Misses      29665    29665

Flag	Coverage Δ
#single	`42.46% <ø> (ø)`	⬆️

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 b45eb26...b3d878d. Read the comment docs.

datapythonista · 2018-11-22T13:45:20Z

Nice. I surely reviewed the flake8 fixing in cookbook.rst, but I'm not sure about the other files. If you can check in the tickets if there has been some work, may be you can restore the fixes (or create new PRs for them).

Let me review the changes, but sounds great. Thanks!

FHaase · 2018-11-22T14:01:14Z

I've seen the cookbook.rst fixes and that @YuechengWu is taking on advanced.rst. io.rst hasn't been touched by myself yet. And other issues/PRs haven't been created yet afaik.

datapythonista

Great changes, thanks for taking care of this @FHaase

I added some comments, questions and ideas, if you can have a look

datapythonista · 2018-11-22T13:48:19Z

doc/source/10min.rst

@@ -84,7 +80,7 @@ will be completed:
 .. ipython::

   @verbatim
-   In [1]: df2.<TAB>
+   In [1]: df2.<TAB>                                         # noqa: E225, E999


In some other PRs we've been adding python to .. ipython:: python and then we could remove the In [1]. Also, not sure if it makes sense to move the @verbatim to :verbatim: under the directive, as I've seen in other places?

datapythonista · 2018-11-22T13:49:52Z

doc/source/10min.rst

@@ -719,7 +719,7 @@ of the columns with labels:
   df = df.cumsum()

   @savefig frame_plot_basic.png
-   plt.figure(); df.plot(); plt.legend(loc='best')
+   plt.figure(); df.plot(); plt.legend(loc='best')                 # noqa: E702


I'd prefer to have one in each line instead of the noqa. Or does it need to be in the same line for the savefig?

@datapythonista If simply moved into single lines the graph remains empty.

@savefig frame_plot_basic.png plt.figure() df.plot() plt.legend(loc='best')

This can be fixed by moving @savfig above the last statement:

plt.figure() df.plot() @savefig frame_plot_basic.png plt.legend(loc='best')

However it becomes really verbose if you compare it to how it is before:

my preferred option would be to use:

plt.figure(); df.plot(); plt.legend(loc='best');

and I'm not sure if the plt.figure() is needed

datapythonista · 2018-11-22T13:52:26Z

doc/source/10min.rst

@@ -793,7 +793,7 @@ Gotchas

 If you are attempting to perform an operation you might see an exception like:

-.. code-block:: python
+.. code-block:: pycon


what does pycon do?

would it make sense to make it an ipython block, for consistency with the rest?

Also, unrelated to your changes, but I'd change the print("I was true") by a pass, I think it add more confusion than value

its the lexer for pygments.lexers.python.PythonConsoleLexer.

But I've changed it to .. ipython:: anyway.

datapythonista · 2018-11-22T13:53:56Z

doc/source/basics.rst

@@ -751,7 +754,7 @@ For example, we can fit a regression using statsmodels. Their API expects a form

 .. ipython:: python

-   import statsmodels.formula.api as sm
+   import statsmodels.formula.api as sm                            # noqa: E401


I guess you checked it, but is the import needed?

doc/source/basics.rst

datapythonista · 2018-11-22T14:10:30Z

doc/source/timeseries.rst

@@ -32,7 +26,10 @@ Parsing time series information from various sources and formats

 .. ipython:: python

-   dti = pd.to_datetime(['1/1/2018', np.datetime64('2018-01-01'), datetime(2018, 1, 1)])
+   from datetime import datetime, timedelta, time


I'd prefer a import datetime

datapythonista · 2018-11-22T14:13:29Z

doc/source/visualization.rst

@@ -2,25 +2,31 @@
 .. _visualization:

 .. ipython:: python
+   :flake8-add-ignore: E702, E703


not sure why this is ignored, I'd prefer to not ignore much, but if needed please add a comment on what is being ignored

datapythonista · 2018-11-22T14:13:44Z

doc/source/visualization.rst

   :suppress:

   import numpy as np
   import pandas as pd
+
+   import matplotlib  # noqa: F401
+   # matplotlib.style.use('default')


Can we remove the import? and the comment?

datapythonista · 2018-11-22T14:14:31Z

doc/source/visualization.rst

 *************
 Visualization
 *************

 We use the standard convention for referencing the matplotlib API:

 .. ipython:: python
+   :flake8-group: None
+   :flake8-set-ignore: F401


if there are unused imports please remove them, and if really needed, better a noqa in the import

datapythonista · 2018-11-22T14:15:02Z

doc/source/visualization.rst

@@ -133,6 +141,8 @@ For example, a bar plot can be created the following way:
 You can also create these other plots using the methods ``DataFrame.plot.<kind>`` instead of providing the ``kind`` keyword argument. This makes it easier to discover plot methods and the specific arguments they use:

 .. ipython::
+    :flake8-group: None
+    :flake8-add-ignore: E999, E225, F821


same, add comments on what is being ignored

FHaase · 2018-11-23T15:27:06Z

plt.figure() and plt.close('all') doesn't make any difference in the generated pages.
There are no windows popping up during python make.py html --single *
So I've chosen to get rid of them.

within 10min.rst and visualization.rst I splitted all statements on seperate lines and suppressed not useful outputs like <matplotlib.table.Table at 0x7efef9e37358>.

datapythonista · 2018-11-24T20:26:25Z

Do you mind explaining again what's the reason for using different directives. Ideally for the code it'd be nice to just have .. ipython:: python blocks. I'm sure there are reasons for the other, but it's not obvious while reviewing why in come cases we use .. ipython:: ipython or ..code-block::pycon.

Also, I'm sorry to ask this, but we need to split this PR. I understand for you it's easier to have everything in a single branch, and it's some extra work now that you already got the work done. But from a reviewers point to view, I'm sure other reviewers will have comments, and having all in the same place means that for even a single trivial change, we need to spend a lot of time reviewing the whole thing.

Would be nice to have the obvious pep8 fixes in a first PR, as they will be merged fast. And adding new dependencies, non trivial changes to the the directives (skipping certain rules...), the upgrade of flake8... in another. Now that you already got all the changes do what you can, and dividing by whole files it's perfectly fine. But for the next time, please consider the reviewing process when making the changes.

Thanks @FHaase

Update flake8-rst version to version 0.6.0+. Signed-off-by: Fabian Haase <[email protected]>

datapythonista · 2018-11-25T22:35:15Z

doc/source/conf.py

@@ -74,6 +74,7 @@
              'sphinx.ext.ifconfig',
              'sphinx.ext.linkcode',
              'nbsphinx',
+              'flake8_rst.sphinxext.custom_roles',


What does this do? I know we haven't done that before, but would be good to have a comment

flake8-rst allows for customized merging code-blocks. Like

.. code-block:: python :flake8-group: GroupForExample#1 code...

However sphinx throws an exception if a directive is decorated by an unknown role.
So all it does is telling the IPython and code-block directive that certain roles can be added.

Would it be a problem for you postponing this to a new PR? I guess that's the best solution, but would like to give it a second thought, as I'm considering to build the docs without sphinx (#23763), and everything that requires a sphinx plugin would be extra work if we do that.

If it's not giving you more work or headaches, I'd prefer to fix everything else for now (may be we can ignore F821 which I think it's the main error avoided by this plugin), and then be back to this. Would that be ok?

datapythonista · 2018-11-25T22:35:58Z

setup.cfg

-    F821,  # undefined name
-    W391,  # blank line at end of file [Seems to be a bug (v0.4.1)]
+ignore = E402,  # module level import not at top of file
+         W503,  # line break before binary operator


aren't we adding the semicolon at the end then?

ignoring W503 allows for

x = (a..... - b....)

So there may be a \n before a -, + ...

ignoring W504? would enforce

x = (a..... - b....)

With a \n after the -, +, ...

the question was if you wanted to ignore the errors for the semicolon in df.plot(); in this PR.

No, I'd rather have it on a per file based setting. It's only useful if there are many plots in one file that create cryptic outputs. If not it's better to have it checked.

datapythonista · 2018-11-25T22:36:43Z

setup.cfg

+    doc/source/text.rst
+    doc/source/timedeltas.rst
+    doc/source/timeseries.rst
+    doc/source/visualization.rst


we can do it later, but I think there are many of these that can now be removed, right?

I'd do it later after giving it a final check.
Some of the files still have issues regarding PEP-8 but they have to be ignored by the newly introduced :flake8-group: role.

I found out that if somewhere in the checked code-block is an E999 issue (even ignored) all issues relying on AST checks like F821 no longer are checked. It's not a problem with flake8 on python code as syntax errors are eventually fixed, but in context of documentation those issues sometimes make sense like in

.. ipython:: @verbatim In [1]: df2.<TAB> df2.A df2.bool df2.abs df2.boxplot df2.add df2.C df2.add_prefix df2.clip

These blocks have to be manually ignored and therefore I'm including the checks after this is merged.

I'm not sure I'm following, but I wouldn't overcomplicate things. I think that could be in its own block with nothing else, and have a noqa for the syntax error, and I guess nothing else would be needed.

Or I think the directive also has a skip or something like that, right?

Yes, however when running flake8-rst on the file, it looks for all the code-blocks (.. code-block::, .. ipython::), removes everything thats not code like (>>>, outputs, ...), and then merges everything into one big block which gets passed over to flake8.

If somewhere in that block is a syntax error, flake8 stops checking for AST-related issues. putting # noqa: E999 only prevents output, but AST checks are still broken.

However the new flake8 version will support to define which blocks are merged together, that way E999 issues can be seperated from the rest of the blocks, basically creating two different checks. One with broken AST, the other with working AST. That way F821 issues remain to be found in the rest of the file. Otherwise flake8-rst wouldn't output anything even when there are issues.

It's surely more complicated than just ignoring E999, but at least it's checking everything.

flake8-rst works on the bare *.rst files, therefore .. ipython related skips have no effect.

Sorry didn't read it correctly:

I think that could be in its own block

Exactly, we could put one of these

:flake8-group: TABOutput + :flake8-add-ignore: E999, E225 and later only reuse :flake8-group: TABOutput

:flake8-group: None + # noqa: E999, E225 or :flake8-add-ignore: E999, E225

:flake8-group: Ignore [disables checking for block]

'flake8_rst.sphinxext.custom_roles', got added to allow these roles.

I'd prefer to use a way without # noqa as it would get rendered in the documentation and might lead to slight confusion for readers who just use pandas and don't use flake8 or similar.

datapythonista · 2018-11-30T13:42:11Z

@FHaase can you check the comments from the previous review?

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

datapythonista · 2018-12-02T13:33:39Z

@FHaase sorry to bother you with one more question. But regarding the blocks that break the syntax, and don't let the rest of the document be generated. Doesn't flake8 skip the blocks marked as :verbatim:?

So, if you don't mind would be great to remove the inclusion of the sphinx extension for now, remove any file that can be removed from the list of ignores, merge master, and leave the CI green. So we can merge this, and continue the discussion while we work on the rest of the .rst pages.

FHaase · 2018-12-02T15:11:37Z

Doesn't flake8 skip the blocks marked as :verbatim:

flake8-rst doesn't skip those.

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

datapythonista

lgtm

thanks @FHaase

jreback · 2018-12-02T18:09:43Z

thanks @FHaase

as followups would be great to remove .rst that already pass (you have done some PR's on this already)

datapythonista · 2018-12-02T18:29:41Z

Opened #24051 to remove the ignored files that have been fixed

datapythonista added Docs Code Style Code style, linting, code_checks Clean labels Nov 22, 2018

FHaase mentioned this pull request Nov 22, 2018

DOC: Fix format of io.rst #23791

Closed

datapythonista reviewed Nov 22, 2018

View reviewed changes

FHaase force-pushed the upgrade-flake8-rst branch from de847b0 to 29039aa Compare November 25, 2018 15:08

Update flake8-rst version

a2cf5ab

Update flake8-rst version to version 0.6.0+. Signed-off-by: Fabian Haase <[email protected]>

FHaase force-pushed the upgrade-flake8-rst branch from 29039aa to a2cf5ab Compare November 25, 2018 15:43

FHaase mentioned this pull request Nov 25, 2018

DOC: Fix PEP-8 issues in 10min.rst #23908

Merged

datapythonista reviewed Nov 25, 2018

View reviewed changes

datapythonista mentioned this pull request Nov 25, 2018

Fix format of advanced.rst according to PEP-8 standard #23893

Merged

4 tasks

This was referenced Nov 28, 2018

Fix build by removing flake8-rst #23975

Closed

Fix PEP-8 issues in contributing.rst #24003

Merged

FHaase added 2 commits November 30, 2018 16:20

Merge branch 'master' into upgrade-flake8-rst

a300944

Add comment for sphinx extension

363d0df

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

FHaase force-pushed the upgrade-flake8-rst branch from e9ec5ce to 363d0df Compare November 30, 2018 16:38

datapythonista mentioned this pull request Dec 2, 2018

DOC: Fix flake8 problems in io.rst #23855

Merged

4 tasks

FHaase added 2 commits December 2, 2018 16:30

Merge branch 'master' into upgrade-flake8-rst

6cebdb0

review of @datapythonista

b3d878d

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

datapythonista approved these changes Dec 2, 2018

View reviewed changes

jreback added this to the 0.24.0 milestone Dec 2, 2018

jreback merged commit 35b1702 into pandas-dev:master Dec 2, 2018

FHaase deleted the upgrade-flake8-rst branch December 2, 2018 19:29

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

DOC: Upgrade flake8-rst version (pandas-dev#23847)

7688aae

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

DOC: Upgrade flake8-rst version (pandas-dev#23847)

9f13cec

DOC: Upgrade flake8-rst version #23847

DOC: Upgrade flake8-rst version #23847

Conversation

FHaase commented Nov 21, 2018 • edited Loading

pep8speaks commented Nov 21, 2018

datapythonista commented Nov 22, 2018

FHaase commented Nov 22, 2018 • edited Loading

codecov bot commented Nov 22, 2018 • edited Loading

Codecov Report

datapythonista commented Nov 22, 2018

FHaase commented Nov 22, 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

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

FHaase commented Nov 23, 2018

datapythonista commented Nov 24, 2018

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

Choose a reason for hiding this comment

datapythonista Nov 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

datapythonista commented Nov 30, 2018

datapythonista commented Dec 2, 2018

FHaase commented Dec 2, 2018 • edited Loading

datapythonista left a comment

Choose a reason for hiding this comment

jreback commented Dec 2, 2018

datapythonista commented Dec 2, 2018

FHaase commented Nov 21, 2018 •

edited

Loading

FHaase commented Nov 22, 2018 •

edited

Loading

codecov bot commented Nov 22, 2018 •

edited

Loading

datapythonista Nov 25, 2018 •

edited

Loading

FHaase commented Dec 2, 2018 •

edited

Loading