Skip to content

DOC: more consistent flake8-commands in contributing.rst #23724

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
Nov 20, 2018
Merged

DOC: more consistent flake8-commands in contributing.rst #23724

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5bcf750
DOC: more consistent flake8-commands in contributing.rst
h-vetinari Nov 15, 2018
c9c64dd
Typo
h-vetinari Nov 15, 2018
87f2edb
Review (datapythonista)
h-vetinari Nov 16, 2018
3d0a2f5
Typo
h-vetinari Nov 16, 2018
54cbc29
Also harmonize makefile lint-diff
h-vetinari Nov 19, 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
19 changes: 10 additions & 9 deletions doc/source/contributing.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -575,7 +575,7 @@ the `flake8 <https://pypi.org/project/flake8>`_ tool
and report any stylistic errors in your code. Therefore, it is helpful before
submitting code to run the check yourself on the diff::

git diff master -u -- "*.py" | flake8 --diff
git diff upstream/master -u -- "pandas/*.py" | flake8 --diff
Copy link
Member

Choose a reason for hiding this comment

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

why "pandas/*.py"? I think we should also check .py files in scripts/, ci/ and everywhere else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@datapythonista
Happy to remove that too, if desired. As the PR stands, this was another inconsistency, and all the other commands (i.a. also the raison-d'être for grep) are filtering to pandas/.

Copy link
Member

Choose a reason for hiding this comment

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

Don't think it's relevant here but want to call out that there can certainly be performance implications to not restricting queries to the pandas directory. For instance, when grepping from the top level if you don't restrict to that folder you could have a command that runs potentially much longer if it starts searching through the asv_bench environments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, but it's only checking the diff anyway, which - even in the asv_bench - will be relatively small (or, for most PRs, non-existent)


This command will catch any stylistic errors in your changes specifically, but
be beware it may not catch all of them. For example, if you delete the only
Expand All @@ -584,21 +584,22 @@ unused function. However, style-checking the diff will not catch this because
the actual import is not part of the diff. Thus, for completeness, you should
run this command, though it will take longer::

git diff master --name-only -- "*.py" | grep "pandas/" | xargs -r flake8
git diff upstream/master --name-only -- "pandas/*.py" | xargs -r flake8

Note that on OSX, the ``-r`` flag is not available, so you have to omit it and
run this slightly modified command::

git diff master --name-only -- "*.py" | grep "pandas/" | xargs flake8
git diff upstream/master --name-only -- "pandas/*.py" | xargs flake8

Windows does not support the ``grep`` and ``xargs`` commands (unless installed
for example via the `MinGW <http://www.mingw.org/>`__ toolchain), but one can
imitate the behaviour as follows::
Windows does not support the ``xargs`` command (unless installed for example
via the `MinGW <http://www.mingw.org/>`__ toolchain), but one can imitate the
behaviour as follows::

for /f %i in ('git diff upstream/master --name-only ^| findstr pandas/') do flake8 %i
for /f %i in ('git diff upstream/master --name-only -- "pandas/*.py"') do flake8 %i

This will also get all the files being changed by the PR (and within the
``pandas/`` folder), and run ``flake8`` on them one after the other.
This will get all the files being changed by the PR (which are also within the
``pandas/`` folder and ending with ``.py``), and run ``flake8`` on them,
one after the other.

.. _contributing.import-formatting:

Expand Down