[MRG+3] Add flake8 linting of the diff in Travis #7127

lesteve · 2016-08-02T13:22:26Z

What does this implement/fix? Explain your changes.

This allows to check that PRs do not add obvious flake8 violations. The main things behind it are:

find common ancestor between branch and scikit-learn/scikit-learn remote
run flake8 --diff on the diff between the branch and the common ancestor

This approach has been used in joblib and nilearn for a little while and works fine.

Additional features:

the line numbers in Travis matches the local branch on the PR author machine
./build_tools/travis/flake8_diff.sh can be run locally for quick turn-around

An example output for a PR adding flake 8 violations:
https://travis-ci.org/joblib/joblib/jobs/149138375

Creating a PR was motivated by https://travis-ci.org/joblib/joblib/jobs/149138375. As I said in this issue:

This way we make sure that no obvious flake8 violations are introduced by PRs while not having to fix all the flake8 violations in the entire source tree, which would avoid the main concern of creating conflicts with existing PRs.

ping @agramfort @GaelVaroquaux

jnothman · 2016-08-02T13:51:59Z

build_tools/travis/flake8_diff.sh

+        # reported by Travis will match with the local code.
+        BRANCH_NAME=travis_pr_$TRAVIS_PULL_REQUEST
+        git fetch $REMOTE pull/$TRAVIS_PULL_REQUEST/head:$BRANCH_NAME
+        git checkout $BRANCH_NAME


I assume there are no concurrency issues with modifying the working copy in the current directory? The rest of this script only modifies .git. And I don't actually see why a working copy is needed.

In my mind these lines were supposed of doing the same as what Travis does by default, but with the branch as it is on the PR author's machine instead of at is is after the merge into master. This way the line numbers on Travis and on the PR author machine match.

I assume there are no concurrency issues with modifying the working copy in the current directory? The rest of this script only modifies .git. And I don't actually see why a working copy is needed.

Note this only happens on Travis (not if you run the command locally) so I am not sure about which concurrency issue you are talking about. It may be true that we may not need a working copy but again this happens only on Travis so is it such a big problem ?

It's not a big problem. I suppose Travis can't reuse working copies for the various tests, so I'm thinking nonsense.

jnothman · 2016-08-02T13:57:59Z

I think this is a good idea. Pity flake8 doesn't cover Cython.

jnothman · 2016-08-02T14:00:34Z

build_tools/travis/flake8_diff.sh

+
+# Conservative approach: diff without context so that code that was
+# not changed does not create failures
+git diff --unified=0 $COMMIT | flake8 --diff --show-source


I assume this takes care to only do things to .py files. I also assume it does not handle rst, pyx, pxd.

I assume this takes care to only do things to .py files. I also assume it does not handle rst, pyx, pxd.

Only .py files are taken into account indeed.

I'm not sure I understand what's happening here. flake8 is only run on the added lines, right? So how can it detect undefined names? Or does it parse the diff and use the whole file as context?

From some tests I ran a long time ago it only uses the diff, I agree that the non PEP8 stuff (e.g. unused variable) is mostly limited to new file (or at least when a significant amount of code is added).

Hm I'm not sure if that is how flake8 --diff is meant to be used? I think you just have to remove the =0 and it does the right thing.

ok never mind. my test show that your line does the right thing, though I have no idea how it works.

jnothman · 2016-08-02T14:01:49Z

LGTM

amueller · 2016-08-02T15:27:20Z

build_tools/travis/install.sh

@@ -53,11 +53,11 @@ if [[ "$DISTRIB" == "conda" ]]; then
    if [[ "$INSTALL_MKL" == "true" ]]; then
        conda create -n testenv --yes python=$PYTHON_VERSION pip nose \
            numpy=$NUMPY_VERSION scipy=$SCIPY_VERSION numpy scipy \
-            cython=$CYTHON_VERSION libgfortran mkl
+            cython=$CYTHON_VERSION libgfortran mkl flake8


not sure if it's worth moving this to the if below because we only need it in the one test? Actually the two kinds of tests have pretty different requirements, right? The flake8 test doesn't even need numpy and scipy I guess? But maybe that doesn't matter so much and we should keep it simple.

Yeah you only need the source code, I went for the smallest change but I agree it could be simplified.

I think the concern here is that if flake8, now or in the future, has other dependencies, then we are installing dependencies that may give us false green CI icons in the future. Seems unlikely, but I don't think it's a great hassle to set this up separately.

I fixed this: now flake8 is only installed when needed (i.e. "$RUN_FLAKE8" == "true").

I don't see that in the code. Can you please point it out?

@jnothman I didn't understand you comment. My concern was more that we are burning travis time by installing numpy, scipy and cython, thought it was a minor concern.

I was saying that when we're actually running tests, installing deps that aren't on our deps list may confound test results.

amueller · 2016-08-02T15:28:31Z

can we get a make target to run that locally (and add it to the developer docs)?

amueller · 2016-08-02T15:37:38Z

build_tools/travis/flake8_diff.sh

@@ -0,0 +1,72 @@
+#!/bin/bash


the file should start with the explanation of what's happening as you gave in this PR, I think.

amueller · 2016-08-02T15:43:17Z

Why don't we use https://flake8-diff.readthedocs.io/en/latest/ instead? Hm I guess it looks a bit abandoned. But it seems like something that should be factored out into it's own project.... (it sounds like you just copied a non-trivial bash script among 3 projects with no shared tracking).

lesteve · 2016-08-02T16:54:43Z

Why don't we use https://flake8-diff.readthedocs.io/en/latest/ instead?

I did not bump into this when I looked for a solution for this problem I have to admit. Will look into it a bit more.

(it sounds like you just copied a non-trivial bash script among 3 projects with no shared tracking).

Yep my brain is the only place the shared tracking is happening at the moment ;-).

amueller · 2016-08-02T18:47:19Z

LGTM but I think a standalone solution at some point would be nice ;)
What does the githook in flake8 do?

lesteve · 2016-08-09T12:02:32Z

@amueller @jnothman I added some explanation in flake8_diff.sh and a flake8-diff target in the Makefile (with a mention in the developer doc). Anything else to do before it can be merged?

NelleV · 2016-08-09T16:17:43Z

build_tools/travis/flake8_diff.sh

+#     ancestor
+#
+# Additional features:
+#   - the line numbers in Travis matches the local branch on the PR


matches -> match

NelleV · 2016-08-09T16:18:42Z

LGTM

with respect to common ancestor of branch and scikit-learn/scikit-learn remote. Add a flake8-diff target in the Makefile and mention it in the doc.

NelleV · 2016-08-27T12:13:04Z

@amueller any more comments on this PR?

lesteve · 2016-08-27T13:15:31Z

Thanks @NelleV for bumping this PR for me :)

jnothman · 2016-08-27T13:43:08Z

And yes, thank you, @NelleV, this will be great to have.

so that the flake8 build stands out more on the Travis website.

jnothman · 2016-08-30T02:58:50Z

LGTM

jnothman · 2016-08-30T02:59:39Z

Now it's time to watch all those PRs get crosses next to them... muhuhahaha

jnothman · 2016-08-30T03:00:10Z

Thanks a lot @lesteve

* Add flake8 linting on the diff with respect to common ancestor of branch and scikit-learn/scikit-learn remote. Add a flake8-diff target in the Makefile and mention it in the doc. * Only install flake8 when needed

NelleV · 2016-11-04T17:47:23Z

build_tools/travis/install.sh

+    python setup.py develop
+fi
+
+if [[ "$RUN_FLAKE8" == "true" ]]; then


Hi @lesteve
I am working on a similar PR for matplotlib, and I was wondering why you used strings and not booleans here?

I know nothing about bash booleans in case they do exist. Is there any clear advantage of using them? Also I guess INSTALL_MKL was used like this so I just copied it.

* Add flake8 linting on the diff with respect to common ancestor of branch and scikit-learn/scikit-learn remote. Add a flake8-diff target in the Makefile and mention it in the doc. * Only install flake8 when needed

jnothman reviewed Aug 2, 2016
View reviewed changes

amueller reviewed Aug 2, 2016
View reviewed changes

lesteve force-pushed the add-flake8-diff branch from 521cad2 to 396f604 Compare August 9, 2016 07:01

NelleV reviewed Aug 9, 2016
View reviewed changes

Add flake8 linting on the diff

867df05

with respect to common ancestor of branch and scikit-learn/scikit-learn remote. Add a flake8-diff target in the Makefile and mention it in the doc.

lesteve force-pushed the add-flake8-diff branch from 396f604 to 867df05 Compare August 9, 2016 16:52

jnothman changed the title ~~[MRG] Add flake8 linting of the diff in Travis~~ [MRG+1] Add flake8 linting of the diff in Travis Aug 27, 2016

lesteve added 2 commits August 29, 2016 11:11

Only install flake8 when needed

294b929

Reorder environment variable definitions in .travis.yml

d04beda

so that the flake8 build stands out more on the Travis website.

jnothman changed the title ~~[MRG+1] Add flake8 linting of the diff in Travis~~ [MRG+3] Add flake8 linting of the diff in Travis Aug 30, 2016

jnothman merged commit 4566251 into scikit-learn:master Aug 30, 2016

lesteve deleted the add-flake8-diff branch August 30, 2016 07:27

NelleV mentioned this pull request Oct 29, 2016

PEP8: making travis run pep8 on diff instead of comparing the total number of violation to be more robust. matplotlib/matplotlib#7358

Closed

NelleV reviewed Nov 4, 2016

View reviewed changes

mkcor mentioned this pull request Jan 7, 2018

Check code style of pull requests automatically stan-dev/pystan2#338

Closed

[MRG+3] Add flake8 linting of the diff in Travis #7127

[MRG+3] Add flake8 linting of the diff in Travis #7127

Conversation

lesteve commented Aug 2, 2016

What does this implement/fix? Explain your changes.

jnothman Aug 2, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnothman commented Aug 2, 2016

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

amueller Aug 2, 2016 • edited Loading

Choose a reason for hiding this comment

jnothman commented Aug 2, 2016

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

amueller commented Aug 2, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amueller commented Aug 2, 2016 • edited Loading

lesteve commented Aug 2, 2016

amueller commented Aug 2, 2016

lesteve commented Aug 9, 2016

Choose a reason for hiding this comment

NelleV commented Aug 9, 2016

NelleV commented Aug 27, 2016

lesteve commented Aug 27, 2016

jnothman commented Aug 27, 2016

jnothman commented Aug 30, 2016

jnothman commented Aug 30, 2016

jnothman commented Aug 30, 2016

Choose a reason for hiding this comment

lesteve Nov 4, 2016 • edited Loading

Choose a reason for hiding this comment

jnothman Aug 2, 2016 •

edited

Loading

amueller Aug 2, 2016 •

edited

Loading

amueller commented Aug 2, 2016 •

edited

Loading

amueller commented Aug 2, 2016 •

edited

Loading

lesteve Nov 4, 2016 •

edited

Loading