Merge pull request #18423 from ganesh-k13/enh_lint

ENH: Lint checks for PR diffs

6 files changed, 156 insertions, 11 deletions

diff --git a/.github/workflows/build_test.yml b/.github/workflows/build_test.yml
index 2f792339b..45a7f9ce0 100644
--- a/.github/workflows/build_test.yml
+++ b/.github/workflows/build_test.yml
@@ -19,6 +19,24 @@ env:
   PYTHON_VERSION: 3.7
 
 jobs:
+  lint:
+    if: "github.repository == 'numpy/numpy' && !contains(github.event.head_commit.message, '[ci skip]') && !contains(github.event.head_commit.message, '[skip ci]') && !contains(github.event.head_commit.message, '[skip github]')"
+    runs-on: ubuntu-latest
+    steps:
+    - uses: actions/checkout@v2
+      with:
+        submodules: recursive
+        fetch-depth: 0
+    - uses: actions/setup-python@v2
+      with:
+        python-version: ${{ env.PYTHON_VERSION }}
+    - name: Install linter requirements
+      run:
+        python -m pip install -r linter_requirements.txt
+    - name: Run linter on PR diff
+      run:
+        python tools/linter.py --branch origin/${{ github.base_ref }}
+
   smoke_test:
     if: "github.repository == 'numpy/numpy' && !contains(github.event.head_commit.message, '[ci skip]') && !contains(github.event.head_commit.message, '[skip ci]') && !contains(github.event.head_commit.message, '[skip github]')"
     runs-on: ubuntu-latest
@@ -33,7 +51,7 @@ jobs:
     - uses: ./.github/actions
 
   basic:
-    needs: smoke_test
+    needs: [smoke_test, lint]
     runs-on: ubuntu-latest
     strategy:
       matrix:
@@ -49,7 +67,7 @@ jobs:
     - uses: ./.github/actions
 
   debug:
-    needs: smoke_test
+    needs: [smoke_test, lint]
     runs-on: ubuntu-20.04
     env:
       USE_DEBUG: 1
@@ -64,7 +82,7 @@ jobs:
     - uses: ./.github/actions
 
   blas64:
-    needs: smoke_test
+    needs: [smoke_test, lint]
     runs-on: ubuntu-latest
     env:
       NPY_USE_BLAS_ILP64: 1
@@ -79,7 +97,7 @@ jobs:
     - uses: ./.github/actions
 
   full:
-    needs: smoke_test
+    needs: [smoke_test, lint]
     runs-on: ubuntu-18.04
     env:
       USE_WHEEL: 1
@@ -97,7 +115,7 @@ jobs:
     - uses: ./.github/actions
 
   benchmark:
-    needs: smoke_test
+    needs: [smoke_test, lint]
     runs-on: ubuntu-latest
     env:
       PYTHONOPTIMIZE: 2
@@ -118,7 +136,7 @@ jobs:
     - uses: ./.github/actions
 
   no_relaxed_strides:
-    needs: smoke_test
+    needs: [smoke_test, lint]
     runs-on: ubuntu-latest
     env:
       NPY_RELAXED_STRIDES_CHECKING: 0
@@ -135,7 +153,7 @@ jobs:
     - uses: ./.github/actions
 
   use_wheel:
-    needs: smoke_test
+    needs: [smoke_test, lint]
     runs-on: ubuntu-latest
     env:
       USE_WHEEL: 1
@@ -151,7 +169,7 @@ jobs:
     - uses: ./.github/actions
 
   no_array_func:
-    needs: smoke_test
+    needs: [smoke_test, lint]
     runs-on: ubuntu-latest
     env:
       NUMPY_EXPERIMENTAL_ARRAY_FUNCTION: 0
@@ -166,7 +184,7 @@ jobs:
     - uses: ./.github/actions
 
   no_openblas:
-    needs: smoke_test
+    needs: [smoke_test, lint]
     runs-on: ubuntu-latest
     env:
       BLAS: None
@@ -184,7 +202,7 @@ jobs:
     - uses: ./.github/actions
 
   pypy37:
-    needs: smoke_test
+    needs: [smoke_test, lint]
     runs-on: ubuntu-latest
     steps:
     - uses: actions/checkout@v2
@@ -197,7 +215,7 @@ jobs:
     - uses: ./.github/actions
 
   sdist:
-    needs: smoke_test
+    needs: [smoke_test, lint]
     runs-on: ubuntu-latest
     env:
       USE_SDIST: 1
diff --git a/azure-pipelines.yml b/azure-pipelines.yml
index d2e251acd..60c3ebf74 100644
--- a/azure-pipelines.yml
+++ b/azure-pipelines.yml
@@ -18,6 +18,24 @@ stages:
 - stage: InitialTests
   jobs:
 
+  - job: Lint
+    condition: and(succeeded(), ne(variables['Build.SourceBranch'], 'refs/heads/master'))  # skip for PR merges
+    pool:
+      vmImage: 'ubuntu-18.04'
+    steps:
+    - task: UsePythonVersion@0
+      inputs:
+        versionSpec: '3.8'
+        addToPath: true
+        architecture: 'x64'
+    - script: >-
+        python -m pip install -r linter_requirements.txt
+      displayName: 'Install tools'
+      failOnStderr: true
+    - script: |
+        python tools/linter.py --branch origin/$(System.PullRequest.TargetBranch)
+      displayName: 'Run Lint Checks'
+      failOnStderr: true
   # Native build is based on gcc flag `-march=native`
   - job: Linux_baseline_native
     pool:
diff --git a/linter_requirements.txt b/linter_requirements.txt
new file mode 100644
index 000000000..b5b49bc8c
--- /dev/null
+++ b/linter_requirements.txt
@@ -0,0 +1,2 @@
+pycodestyle==2.5.0
+GitPython==3.1.13
\ No newline at end of file
diff --git a/runtests.py b/runtests.py
index 4c9493a35..5885d2df6 100755
--- a/runtests.py
+++ b/runtests.py
@@ -28,6 +28,12 @@ Generate C code coverage listing under build/lcov/:
     $ python runtests.py --gcov [...other args...]
     $ python runtests.py --lcov-html
 
+Run lint checks.
+Provide target branch name or `uncommitted` to check before committing:
+
+    $ python runtests.py --lint main
+    $ python runtests.py --lint uncommitted
+
 """
 #
 # This is a generic test runner script for projects using NumPy's test
@@ -84,6 +90,10 @@ def main(argv):
     parser.add_argument("--coverage", action="store_true", default=False,
                         help=("report coverage of project code. HTML output goes "
                               "under build/coverage"))
+    parser.add_argument("--lint", default=None,
+                        help="'<Target Branch>' or 'uncommitted', passed to "
+                             "tools/linter.py [--branch BRANCH] "
+                             "[--uncommitted]")
     parser.add_argument("--durations", action="store", default=-1, type=int,
                         help=("Time N slowest tests, time all if 0, time none if < 0"))
     parser.add_argument("--gcov", action="store_true", default=False,
@@ -162,6 +172,9 @@ def main(argv):
         print("*** Benchmarks should not be run against debug "
               "version; remove -g flag ***")
 
+    if args.lint:
+        check_lint(args.lint)
+
     if not args.no_build:
         # we need the noarch path in case the package is pure python.
         site_dir, site_dir_noarch = build_project(args)
@@ -637,6 +650,24 @@ def lcov_generate():
     else:
         print("HTML output generated under build/lcov/")
 
+def check_lint(lint_args):
+    """
+    Adds ROOT_DIR to path and performs lint checks.
+    This functions exits the program with status code of lint check.
+    """
+    sys.path.append(ROOT_DIR)
+    try:
+        from tools.linter import DiffLinter
+    except ModuleNotFoundError as e:
+        print(f"Error: {e.msg}. "
+              "Install using linter_requirements.txt.")
+        sys.exit(1)
+
+    uncommitted = lint_args == "uncommitted"
+    branch = "main" if uncommitted else lint_args
+
+    DiffLinter(branch).run_lint(uncommitted)
+
 
 if __name__ == "__main__":
     main(argv=sys.argv[1:])
diff --git a/tools/lint_diff.ini b/tools/lint_diff.ini
new file mode 100644
index 000000000..ba091468e
--- /dev/null
+++ b/tools/lint_diff.ini
@@ -0,0 +1,5 @@
+[pycodestyle]
+max_line_length = 79
+statistics = True
+ignore = E121,E122,E123,E125,E126,E127,E128,E226,E251,E265,E266,E302,E402,E712,E721,E731,E741,W291,W293,W391,W503,W504
+exclude = numpy/__config__.py
diff --git a/tools/linter.py b/tools/linter.py
new file mode 100644
index 000000000..2952e91ed
--- /dev/null
+++ b/tools/linter.py
@@ -0,0 +1,71 @@
+import os
+import sys
+import subprocess
+from argparse import ArgumentParser
+from git import Repo, exc
+
+CONFIG = os.path.join(
+         os.path.abspath(os.path.dirname(__file__)),
+         'lint_diff.ini',
+)
+
+
+class DiffLinter:
+    def __init__(self, branch):
+        self.branch = branch
+        self.repo = Repo('.')
+        self.head = self.repo.head.commit
+
+    def get_branch_diff(self, uncommitted = False):
+        """
+            Determine the first common ancestor commit.
+            Find diff between branch and FCA commit.
+            Note: if `uncommitted` is set, check only
+                  uncommitted changes
+        """
+        try:
+            commit = self.repo.merge_base(self.branch, self.head)[0]
+        except exc.GitCommandError:
+            print(f"Branch with name `{self.branch}` does not exist")
+            sys.exit(1)
+
+        if uncommitted:
+            diff = self.repo.git.diff(self.head, '--unified=0', '***.py')
+        else:
+            diff = self.repo.git.diff(commit, self.head,
+                   '--unified=0', '***.py')
+        return diff
+
+    def run_pycodestyle(self, diff):
+        """
+            Original Author: Josh Wilson (@person142)
+            Source:
+              https://github.com/scipy/scipy/blob/master/tools/lint_diff.py
+            Run pycodestyle on the given diff.
+        """
+        res = subprocess.run(
+            ['pycodestyle', '--diff', '--config', CONFIG],
+            input=diff,
+            stdout=subprocess.PIPE,
+            encoding='utf-8',
+        )
+        return res.returncode, res.stdout
+
+    def run_lint(self, uncommitted):
+        diff = self.get_branch_diff(uncommitted)
+        retcode, errors = self.run_pycodestyle(diff)
+
+        errors and print(errors)
+
+        sys.exit(retcode)
+
+
+if __name__ == '__main__':
+    parser = ArgumentParser()
+    parser.add_argument("--branch", type=str, default='main',
+                        help="The branch to diff against")
+    parser.add_argument("--uncommitted", action='store_true',
+                        help="Check only uncommitted changes")
+    args = parser.parse_args()
+
+    DiffLinter(args.branch).run_lint(args.uncommitted)