[MRG+3] Add flake8 linting of the diff in Travis (scikit-learn#7127)

* 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

Author: lesteve

.travis.yml

@@ -42,6 +42,12 @@ env:
     - DISTRIB="conda" PYTHON_VERSION="3.5" INSTALL_MKL="true"
       NUMPY_VERSION="1.10.4" SCIPY_VERSION="0.17.0" CYTHON_VERSION="0.23.4"
       CACHED_BUILD_DIR="$HOME/sklearn_build_latest"
+    # flake8 linting on diff wrt common ancestor with upstream/master
+    - RUN_FLAKE8="true" SKIP_TESTS="true"
+      DISTRIB="conda" PYTHON_VERSION="3.5" INSTALL_MKL="true"
+      NUMPY_VERSION="1.10.4" SCIPY_VERSION="0.17.0" CYTHON_VERSION="0.23.4"
+      CACHED_BUILD_DIR="$HOME/dummy"
+
 
 matrix:
   allow_failures:

Makefile

@@ -68,3 +68,6 @@ doc-noplot: inplace
 code-analysis:
 	flake8 sklearn | grep -v __init__ | grep -v external
 	pylint -E -i y sklearn/ -d E1103,E0611,E1101
+
+flake8-diff:
+	./build_tools/travis/flake8_diff.sh

build_tools/travis/flake8_diff.sh

@@ -0,0 +1,85 @@
+#!/bin/bash
+
+# This script is used in Travis to check that PRs do not add obvious
+# flake8 violations. It relies on two things:
+#   - find common ancestor between branch and
+#     scikit-learn/scikit-learn remote
+#   - run flake8 --diff on the diff between the branch and the common
+#     ancestor
+#
+# Additional features:
+#   - the line numbers in Travis match the local branch on the PR
+#     author machine.
+#   - ./build_tools/travis/flake8_diff.sh can be run locally for quick
+#     turn-around
+
+set -e
+
+PROJECT=scikit-learn/scikit-learn
+PROJECT_URL=https://github.com/$PROJECT.git
+
+echo "Remotes:"
+git remote --verbose
+
+# Find the remote with the project name (upstream in most cases)
+REMOTE=$(git remote -v | grep $PROJECT | cut -f1 | head -1)
+
+# Add a temporary remote if needed. For example this is necessary when
+# Travis is configured to run in a fork. In this case 'origin' is the
+# fork and not the reference repo we want to diff against.
+if [[ -z "$REMOTE" ]]; then
+    TMP_REMOTE=tmp_reference_upstream
+    REMOTE=$TMP_REMOTE
+    git remote add $REMOTE $PROJECT_URL
+fi
+
+if [[ "$TRAVIS" == "true" ]]; then
+    if [[ "$TRAVIS_PULL_REQUEST" == "false" ]]
+    then
+        # Travis does the git clone with a limited depth (50 at the time of
+        # writing). This may not be enough to find the common ancestor with
+        # $REMOTE/master so we unshallow the git checkout
+        git fetch --unshallow || echo "Unshallowing the git checkout failed"
+    else
+        # We want to fetch the code as it is in the PR branch and not
+        # the result of the merge into master. This way line numbers
+        # 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
+    fi
+fi
+
+
+echo -e '\nLast 2 commits:'
+echo '--------------------------------------------------------------------------------'
+git log -2 --pretty=short
+
+git fetch $REMOTE master
+REMOTE_MASTER_REF="$REMOTE/master"
+
+# Find common ancestor between HEAD and remotes/$REMOTE/master
+COMMIT=$(git merge-base @ $REMOTE_MASTER_REF) || \
+    echo "No common ancestor found for $(git show @ -q) and $(git show $REMOTE_MASTER_REF -q)"
+
+if [[ -n "$TMP_REMOTE" ]]; then
+    git remote remove $TMP_REMOTE
+fi
+
+if [ -z "$COMMIT" ]; then
+    exit 1
+fi
+
+echo -e "\nCommon ancestor between HEAD and $REMOTE_MASTER_REF is:"
+echo '--------------------------------------------------------------------------------'
+git show --no-patch $COMMIT
+
+echo -e '\nRunning flake8 on the diff in the range'\
+     "$(git rev-parse --short $COMMIT)..$(git rev-parse --short @)" \
+     "($(git rev-list $COMMIT.. | wc -l) commit(s)):"
+echo '--------------------------------------------------------------------------------'
+
+# 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
+echo -e "No problem detected by flake8\n"

build_tools/travis/install.sh

@@ -53,7 +53,7 @@ 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
     else
         conda create -n testenv --yes python=$PYTHON_VERSION pip nose \
             numpy=$NUMPY_VERSION scipy=$SCIPY_VERSION cython=$CYTHON_VERSION \
@@ -95,18 +95,26 @@ if [[ "$COVERAGE" == "true" ]]; then
     pip install coverage coveralls
 fi
 
-if [ ! -d "$CACHED_BUILD_DIR" ]; then
-    mkdir -p $CACHED_BUILD_DIR
-fi
+if [[ "$SKIP_TESTS" == "true" ]]; then
+    echo "No need to build scikit-learn when not running the tests"
+else
+    if [ ! -d "$CACHED_BUILD_DIR" ]; then
+        mkdir -p $CACHED_BUILD_DIR
+    fi
 
-rsync -av --exclude '.git/' --exclude='testvenv/' \
-      $TRAVIS_BUILD_DIR $CACHED_BUILD_DIR
+    rsync -av --exclude '.git/' --exclude='testvenv/' \
+          $TRAVIS_BUILD_DIR $CACHED_BUILD_DIR
 
-cd $CACHED_BUILD_DIR/scikit-learn
+    cd $CACHED_BUILD_DIR/scikit-learn
 
-# Build scikit-learn in the install.sh script to collapse the verbose
-# build output in the travis output when it succeeds.
-python --version
-python -c "import numpy; print('numpy %s' % numpy.__version__)"
-python -c "import scipy; print('scipy %s' % scipy.__version__)"
-python setup.py develop
+    # Build scikit-learn in the install.sh script to collapse the verbose
+    # build output in the travis output when it succeeds.
+    python --version
+    python -c "import numpy; print('numpy %s' % numpy.__version__)"
+    python -c "import scipy; print('scipy %s' % scipy.__version__)"
+    python setup.py develop
+fi
+
+if [[ "$RUN_FLAKE8" == "true" ]]; then
+    conda install flake8
+fi

build_tools/travis/test_script.sh

@@ -8,32 +8,42 @@
 
 set -e
 
-# Get into a temp directory to run test from the installed scikit learn and
-# check if we do not leave artifacts
-mkdir -p $TEST_DIR
-# We need the setup.cfg for the nose settings
-cp setup.cfg $TEST_DIR
-cd $TEST_DIR
-
 python --version
 python -c "import numpy; print('numpy %s' % numpy.__version__)"
 python -c "import scipy; print('scipy %s' % scipy.__version__)"
 python -c "import multiprocessing as mp; print('%d CPUs' % mp.cpu_count())"
 
-# Skip tests that require large downloads over the network to save bandwidth
-# usage as travis workers are stateless and therefore traditional local
-# disk caching does not work.
-export SKLEARN_SKIP_NETWORK_TESTS=1
-
-if [[ "$COVERAGE" == "true" ]]; then
-   nosetests -s --with-coverage --with-timer --timer-top-n 20 sklearn
-else
-   nosetests -s --with-timer --timer-top-n 20 sklearn
+run_tests() {
+    # Get into a temp directory to run test from the installed scikit learn and
+    # check if we do not leave artifacts
+    mkdir -p $TEST_DIR
+    # We need the setup.cfg for the nose settings
+    cp setup.cfg $TEST_DIR
+    cd $TEST_DIR
+
+    # Skip tests that require large downloads over the network to save bandwidth
+    # usage as travis workers are stateless and therefore traditional local
+    # disk caching does not work.
+    export SKLEARN_SKIP_NETWORK_TESTS=1
+
+    if [[ "$COVERAGE" == "true" ]]; then
+        nosetests -s --with-coverage --with-timer --timer-top-n 20 sklearn
+    else
+        nosetests -s --with-timer --timer-top-n 20 sklearn
+    fi
+
+    # Is directory still empty ?
+    ls -ltra
+
+    # Test doc
+    cd $CACHED_BUILD_DIR/scikit-learn
+    make test-doc test-sphinxext
+}
+
+if [[ "$RUN_FLAKE8" == "true" ]]; then
+    source build_tools/travis/flake8_diff.sh
 fi
 
-# Is directory still empty ?
-ls -ltra
-
-# Test doc
-cd $CACHED_BUILD_DIR/scikit-learn
-make test-doc test-sphinxext
+if [[ "$SKIP_TESTS" != "true" ]]; then
+    run_tests
+fi

doc/developers/contributing.rst

@@ -157,7 +157,10 @@ Contributing pull requests
 It is recommended to check that your contribution complies with the following
 rules before submitting a pull request:
 
-    * Follow the `coding-guidelines`_ (see below).
+    * Follow the `coding-guidelines`_ (see below). To make sure that
+      your PR does not add PEP8 violations you can run
+      `./build_tools/travis/flake8_diff.sh` or `make flake8-diff` on a
+      Unix-like system.
 
     * When applicable, use the validation tools and other code in the
       ``sklearn.utils`` submodule.  A list of utility routines available