Skip to content

DOC: Upgrade flake8-rst version #23847

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Dec 2, 2018
Merged

DOC: Upgrade flake8-rst version #23847

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a2cf5ab
Update flake8-rst version
FHaase Nov 16, 2018
a300944
Merge branch 'master' into upgrade-flake8-rst
FHaase Nov 30, 2018
363d0df
Add comment for sphinx extension
FHaase Nov 30, 2018
6cebdb0
Merge branch 'master' into upgrade-flake8-rst
FHaase Dec 2, 2018
b3d878d
review of @datapythonista
FHaase Dec 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ci/deps/travis-36.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ dependencies:
- fastparquet
- flake8>=3.5
- flake8-comprehensions
- flake8-rst=0.4.2
- flake8-rst>=0.6.0
- gcsfs
- geopandas
- html5lib
Expand Down
2 changes: 1 addition & 1 deletion environment.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ dependencies:
- cython>=0.28.2
- flake8
- flake8-comprehensions
- flake8-rst=0.4.2
- flake8-rst>=0.6.0
- gitpython
- hypothesis>=3.82
- isort
Expand Down
2 changes: 1 addition & 1 deletion requirements-dev.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ pytz
cython>=0.28.2
flake8
flake8-comprehensions
flake8-rst==0.4.2
flake8-rst>=0.6.0
gitpython
hypothesis>=3.82
isort
Expand Down
46 changes: 43 additions & 3 deletions setup.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,25 +31,65 @@ exclude =
env # exclude asv benchmark environments from linting

[flake8-rst]
ignore =
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

exclude =
doc/source/whatsnew/v0.7.0.rst
doc/source/whatsnew/v0.7.3.rst
doc/source/whatsnew/v0.8.0.rst
doc/source/whatsnew/v0.9.0.rst
doc/source/whatsnew/v0.9.1.rst
doc/source/whatsnew/v0.10.0.rst
doc/source/whatsnew/v0.10.1.rst
doc/source/whatsnew/v0.11.0.rst
doc/source/whatsnew/v0.12.0.rst
doc/source/whatsnew/v0.13.0.rst
doc/source/whatsnew/v0.13.1.rst
doc/source/whatsnew/v0.14.0.rst
doc/source/whatsnew/v0.14.1.rst
doc/source/whatsnew/v0.15.0.rst
doc/source/whatsnew/v0.15.1.rst
doc/source/whatsnew/v0.15.2.rst
doc/source/whatsnew/v0.16.0.rst
doc/source/whatsnew/v0.16.1.rst
doc/source/whatsnew/v0.16.2.rst
doc/source/whatsnew/v0.17.0.rst
doc/source/whatsnew/v0.17.1.rst
doc/source/whatsnew/v0.18.0.rst
doc/source/whatsnew/v0.18.1.rst
doc/source/whatsnew/v0.19.0.rst
doc/source/whatsnew/v0.20.0.rst
doc/source/whatsnew/v0.21.0.rst
doc/source/whatsnew/v0.22.0.rst
doc/source/whatsnew/v0.23.0.rst
doc/source/whatsnew/v0.23.1.rst
doc/source/whatsnew/v0.23.2.rst
doc/source/whatsnew/v0.24.0.rst
doc/source/10min.rst
doc/source/advanced.rst
doc/source/basics.rst
doc/source/categorical.rst
doc/source/comparison_with_r.rst
doc/source/comparison_with_sql.rst
doc/source/comparison_with_stata.rst
doc/source/computation.rst
doc/source/contributing.rst
doc/source/contributing_docstring.rst
doc/source/dsintro.rst
doc/source/enhancingperf.rst
doc/source/extending.rst
doc/source/groupby.rst
doc/source/indexing.rst
doc/source/io.rst
doc/source/merging.rst
doc/source/missing_data.rst
doc/source/options.rst
doc/source/release.rst
doc/source/reshaping.rst
doc/source/timedeltas.rst
doc/source/timeseries.rst
doc/source/visualization.rst
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@datapythonista datapythonista Nov 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.



[yapf]
based_on_style = pep8
Expand Down