Adds pylint on only changed files. (#3038)

.github/workflows/ci.yml

@@ -95,7 +95,6 @@ jobs:
         run: |
           pip install -r requirements.txt
           pip install -r dev_tools/conf/pip-list-dev-tools.txt
-          git config --global user.name ${GITHUB_ACTOR}
       - name: Pytest check
         run: check/pytest --ignore=cirq/contrib --benchmark-skip --actually-quiet
   build_docs:
@@ -145,7 +144,6 @@ jobs:
           pip install -r requirements.txt
           pip install -r cirq/contrib/contrib-requirements.txt
           pip install -r dev_tools/conf/pip-list-dev-tools.txt
-          git config --global user.name ${GITHUB_ACTOR}
       - name: Coverage check
         run: check/pytest-and-incremental-coverage --actually-quiet
   windows:
@@ -177,6 +175,5 @@ jobs:
         run: |
           pip install -r requirements.txt
           pip install -r dev_tools/conf/pip-list-dev-tools.txt
-          git config --global user.name ${GITHUB_ACTOR}
       - name: Pytest check
         run: check/pytest --ignore=cirq/contrib --benchmark-skip

check/all

@@ -17,12 +17,13 @@
 # Usage:
 #     check/all revision [BASE_REVISION] [--only-changed-files] [--apply-format-changes]
 #
-# BASE_REVISION is forwarded to format-incremental and to the pytest checks
-# (pytest-and-incremental-coverage or pytest-changed-files). See those
-# checks for how to specify this value.
+# BASE_REVISION is forwarded to format-incremental and to the pytest and pylint
+# checks (pytest-and-incremental-coverage or pytest-changed-files and
+# pylint-changed-files). See those checks for how to specify this value.
 #
 # If --only-changed-files is specified, pytest-changed-files will be run
-# instead of pytest-and-incremental-coverage.
+# instead of pytest-and-incremental-coverage, and pylint-changed-files will
+# be run instead of pylint.
 #
 # If --apply-format-changes is specified the --apply flag will be passed
 # to format-incremental to apply the format changes suggested by the
@@ -39,7 +40,7 @@ only_changed=0
 rev=""
 for arg in $@; do
     if [[ "${arg}" == "--only-changed-files" ]]; then
-        only_change=1
+        only_changed=1
     elif [[ "${arg}" == "--apply-format-changes" ]]; then
         apply_arg="--apply"
     elif [ -z "${rev}" ]; then
@@ -57,16 +58,21 @@ done
 echo "Running misc"
 check/misc
 
-echo "Running pylint"
-check/pylint
+if [ ${only_changed} -ne 0 ]; then
+  echo "Running incremental pylint"
+  check/pylint-changed-files
+else
+  echo "Running pylint"
+  check/pylint
+fi
 
 echo "Running mypy"
 check/mypy
 
 echo "Running incremental format"
 check/format-incremental "${rev}" "${apply_arg}"
 
-if [ -z "${only_changed}" ]; then
+if [ ${only_changed} -ne 0 ]; then
     echo "Running pytest and incremental coverage on changed files"
     check/pytest-changed-files-and-incremental-coverage "${rev}"
 else

check/pylint

@@ -4,7 +4,7 @@
 # Runs pylint on the repository using a preconfigured .pylintrc file.
 #
 # Usage:
-#     check/pylint [--flags]
+#     check/pylint [--flags for pylint]
 ################################################################################
 
 # Get the working directory to the repo root.

check/pylint-changed-files

@@ -0,0 +1,68 @@
+#!/usr/bin/env bash
+
+################################################################################
+# Runs pylint on changed files using a preconfigured .pylintrc file.
+#
+# Usage:
+#     check/pylint-changed-files [BASE_REVISION]
+#
+# You can specify a base git revision to compare against (i.e. to use when
+# determining whether or not a line is considered to have "changed"). To make
+# the tool more consistent, it actually diffs against the most recent common
+# ancestor of the specified id and HEAD. So if you choose 'origin/master' you're
+# actually diffing against the output of 'git merge-base origin/master HEAD'.
+#
+# If you don't specify a base revision, the following defaults will be tried,
+# in order, until one exists:
+#
+#     1. upstream/master
+#     2. origin/master
+#     3. master
+#
+# If none exists, the script fails.
+#
+# See check/pylint for linting all files.
+################################################################################
+
+# Get the working directory to the repo root.
+cd "$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
+cd "$(git rev-parse --show-toplevel)"
+
+# Figure out which revision to compare against.
+if [ ! -z "$1" ] && [[ $1 != -* ]]; then
+    if [ "$(git cat-file -t $1 2> /dev/null)" != "commit" ]; then
+        echo -e "\033[31mNo revision '$1'.\033[0m" >&2
+        exit 1
+    fi
+    rev=$1
+elif [ "$(git cat-file -t upstream/master 2> /dev/null)" == "commit" ]; then
+    rev=upstream/master
+elif [ "$(git cat-file -t origin/master 2> /dev/null)" == "commit" ]; then
+    rev=origin/master
+elif [ "$(git cat-file -t master 2> /dev/null)" == "commit" ]; then
+    rev=master
+else
+    echo -e "\033[31mNo default revision found to compare against. Argument #1 must be what to diff against (e.g. 'origin/master' or 'HEAD~1').\033[0m" >&2
+    exit 1
+fi
+base="$(git merge-base ${rev} HEAD)"
+if [ "$(git rev-parse ${rev})" == "${base}" ]; then
+    echo -e "Comparing against revision '${rev}'." >&2
+else
+    echo -e "Comparing against revision '${rev}' (merge base ${base})." >&2
+    rev="${base}"
+fi
+
+changed=$(git diff --name-only ${rev} -- \
+    | grep -E "^(cirq|dev_tools|examples).*"
+)
+
+num_changed=$(echo -e "${changed[@]}" | wc -w)
+
+# Run it.
+echo "Found ${num_changed} lintable files associated with changes." >&2
+if [ "${num_changed}" -eq 0 ]; then
+    exit 0
+fi
+pylint --rcfile=dev_tools/conf/.pylintrc ${changed[@]}
+

dev_tools/bash_scripts_test.py

@@ -64,6 +64,8 @@ def run(
 dir=$(git rev-parse --show-toplevel)
 cd {}
 git init --quiet
+git config --local user.name 'Me'
+git config --local user.email '<>'
 git commit -m init --allow-empty --quiet --no-gpg-sign
 {}
 chmod +x ./test-script.sh
@@ -260,6 +262,8 @@ def test_pytest_changed_files_branch_selection(tmpdir_factory):
                  setup='mkdir alt\n'
                  'cd alt\n'
                  'git init --quiet\n'
+                 'git config --local user.name \'Me\'\n'
+                 'git config --local user.email \'<>\'\n'
                  'git commit -m tes --quiet --allow-empty --no-gpg-sign\n'
                  'cd ..\n'
                  'git remote add origin alt\n'
@@ -506,3 +510,77 @@ def test_incremental_format_branch_selection(tmpdir_factory):
         '\x1b[31mSome formatting needed on changed lines\x1b[0m.\n')
     assert result.err.startswith(
         "Comparing against revision 'master' (merge base ")
+
+
+@only_on_posix
+def test_pylint_changed_files_file_selection(tmpdir_factory):
+
+    result = run(script_file='check/pylint-changed-files',
+                 tmpdir_factory=tmpdir_factory,
+                 arg='HEAD~1',
+                 setup='touch file.py\n'
+                 'git add -A\n'
+                 'git commit -m test --quiet --no-gpg-sign\n')
+    assert result.exit_code == 0
+    assert result.out == ''
+    assert result.err.split() == (
+        "Comparing against revision 'HEAD~1'.\n"
+        "Found 0 lintable files associated with changes.\n").split()
+
+    intercepted_prefix = 'INTERCEPTED pylint --rcfile=dev_tools/conf/.pylintrc '
+
+    result = run(script_file='check/pylint-changed-files',
+                 tmpdir_factory=tmpdir_factory,
+                 arg='HEAD~1',
+                 setup='mkdir cirq\n'
+                 'touch cirq/file.py\n'
+                 'git add -A\n'
+                 'git commit -m test --quiet --no-gpg-sign\n')
+    assert result.exit_code == 0
+    assert result.out == intercepted_prefix + 'cirq/file.py\n'
+    assert result.err.split() == (
+        "Comparing against revision 'HEAD~1'.\n"
+        "Found 1 lintable files associated with changes.\n").split()
+
+    result = run(script_file='check/pylint-changed-files',
+                 tmpdir_factory=tmpdir_factory,
+                 arg='HEAD~1',
+                 setup='mkdir cirq\n'
+                 'touch ignore.py cirq/file.py\n'
+                 'git add -A\n'
+                 'git commit -m test --quiet --no-gpg-sign\n')
+    assert result.exit_code == 0
+    assert result.out == intercepted_prefix + 'cirq/file.py\n'
+    assert result.err.split() == (
+        "Comparing against revision 'HEAD~1'.\n"
+        "Found 1 lintable files associated with changes.\n").split()
+
+    result = run(script_file='check/pylint-changed-files',
+                 tmpdir_factory=tmpdir_factory,
+                 arg='HEAD',
+                 setup='mkdir cirq\n'
+                 'touch ignore.py cirq/file.py\n'
+                 'git add -A\n'
+                 'git commit -m test --quiet --no-gpg-sign\n'
+                 'echo x > cirq/file.py')
+    assert result.exit_code == 0
+    assert result.out == intercepted_prefix + 'cirq/file.py\n'
+    assert result.err.split() == (
+        "Comparing against revision 'HEAD'.\n"
+        "Found 1 lintable files associated with changes.\n").split()
+
+    result = run(script_file='check/pylint-changed-files',
+                 tmpdir_factory=tmpdir_factory,
+                 arg='HEAD~1',
+                 setup='mkdir cirq dev_tools examples ignore\n'
+                 'touch cirq/file.py dev_tools/file.py examples/file.y\n'
+                 'touch ignore/ignore.py\n'
+                 'git add -A\n'
+                 'git commit -m test --quiet --no-gpg-sign\n')
+    print(result)
+    assert result.exit_code == 0
+    assert result.out == intercepted_prefix + ('cirq/file.py dev_tools/file.py '
+                                               'examples/file.y\n')
+    assert result.err.split() == (
+        "Comparing against revision 'HEAD~1'.\n"
+        "Found 3 lintable files associated with changes.\n").split()