Lint with flake8

appveyor.yml

@@ -23,6 +23,8 @@ environment:
 init:
  - ps: |
      # iex ((new-object net.webclient).DownloadString('https://raw.githubusercontent.com/appveyor/ci/master/scripts/enable-rdp.ps1'))
+     $pythonVersion = (py --version)
+     echo $pythonVersion
      if ($env:APPVEYOR_REPO_TAG_NAME -and $env:APPVEYOR_REPO_TAG_NAME.StartsWith("release-")) {
       # Strip "release-" prefix.
       $version = $env:APPVEYOR_REPO_TAG_NAME.Substring(8)
@@ -121,10 +123,11 @@ build_script:
 
 before_test:
  # install required packages
- - py -m pip install -r tests/system/requirements.txt
+ - py -m pip install -r tests/system/requirements.txt -r tests/lint/lintInstall/requirements.txt
  - mkdir testOutput
  - mkdir testOutput\unit
  - mkdir testOutput\system
+ - mkdir testOutput\lint
  - ps: |
      $errorCode=0
      $nvdaLauncherFile=".\output\nvda"
@@ -157,6 +160,28 @@ test_script:
      $wc = New-Object 'System.Net.WebClient'
      $wc.UploadFile("https://ci.appveyor.com/api/testresults/junit/$($env:APPVEYOR_JOB_ID)", $unitTestsXml)
      if($errorCode -ne 0) { $host.SetShouldExit($errorCode) }
+ - ps: |
+    if($env:APPVEYOR_PULL_REQUEST_NUMBER) {
+      $lintOutput = (Resolve-Path .\testOutput\lint\)
+      $lintSource = (Resolve-Path .\tests\lint\)
+      git fetch -q origin $env:APPVEYOR_REPO_BRANCH
+      $prDiff = "$lintOutput\prDiff.patch"
+      git diff -U0 FETCH_HEAD...HEAD > $prDiff
+      $flake8Config = "$lintSource\flake8.ini"
+      $flake8Output = "$lintOutput\PR-Flake8.txt"
+      type "$prDiff" | py -m flake8 --diff --output-file="$flake8Output" --tee --config="$flake8Config"
+      if($LastExitCode -ne 0) {
+       $errorCode=$LastExitCode
+       Add-AppveyorMessage "PR introduces Flake8 errors"
+      }
+      Push-AppveyorArtifact $flake8Output
+      $junitXML = "$lintOutput\PR-Flake8.xml"
+      py "$lintSource\createJunitReport.py" "$flake8Output" "$junitXML"
+      Push-AppveyorArtifact $junitXML
+      $wc = New-Object 'System.Net.WebClient'
+      $wc.UploadFile("https://ci.appveyor.com/api/testresults/junit/$($env:APPVEYOR_JOB_ID)", $junitXML)
+      if($errorCode -ne 0) { $host.SetShouldExit($errorCode) }
+    }
  - ps: |
      $testOutput = (Resolve-Path .\testOutput\)
      $systemTestOutput = (Resolve-Path "$testOutput\system")
@@ -169,9 +194,10 @@ test_script:
      $wc.UploadFile("https://ci.appveyor.com/api/testresults/junit/$($env:APPVEYOR_JOB_ID)", (Resolve-Path "$systemTestOutput\systemTests.xml"))
      if($errorCode -ne 0) { $host.SetShouldExit($errorCode) }
 
-artifacts:
- - path: output\*
- - path: output\*\*
+on_finish:
+ - ps: |
+      Get-ChildItem output\*   | % { Push-AppveyorArtifact $_.FullName -FileName $_.Name }
+      Get-ChildItem output\*\* | % { Push-AppveyorArtifact $_.FullName -FileName $_.Name }
 
 deploy_script:
  - ps: |
@@ -180,9 +206,9 @@ deploy_script:
       # Notify our server.
       $exe = Get-ChildItem -Name output\*.exe
       $hash = (Get-FileHash "output\$exe" -Algorithm SHA1).Hash.ToLower()
-      $apiVersion = (python -c "import sys; sys.path.append('source'); from addonAPIVersion import CURRENT; print('{}.{}.{}'.format(*CURRENT))")
+      $apiVersion = (py -c "import sys; sys.path.append('source'); from addonAPIVersion import CURRENT; print('{}.{}.{}'.format(*CURRENT))")
       echo apiversion: $apiVersion
-      $apiCompatTo = (python -c "import sys; sys.path.append('source'); from addonAPIVersion import BACK_COMPAT_TO; print('{}.{}.{}'.format(*BACK_COMPAT_TO))")
+      $apiCompatTo = (py -c "import sys; sys.path.append('source'); from addonAPIVersion import BACK_COMPAT_TO; print('{}.{}.{}'.format(*BACK_COMPAT_TO))")
       echo apiBackCompatTo: $apiCompatTo
       $data = @{
        jobId=$env:APPVEYOR_JOB_ID;

readme.md

@@ -90,7 +90,13 @@ Additionally, the following build time dependencies are included in Git submodul
 * [Boost Optional (stand-alone header)](https://github.com/akrzemi1/Optional), from commit [3922965](https://github.com/akrzemi1/Optional/commit/3922965396fc455c6b1770374b9b4111799588a9)
 
 ### Other Dependencies
-These dependencies are not included in Git submodules, but aren't needed by most people.
+To lint using Flake 8 locally using our SCons integration, some dependencies are installed (automatically) via pip.
+Although this [must be run manually](#linting-your-changes), developers may wish to first configure a Python Virtual Environment to ensure their general install is not affected.
+* Flake8
+* Flake8-tabs
+
+
+The following dependencies aren't needed by most people, and are not included in Git submodules:
 
 * To generate developer documentation for nvdaHelper: [Doxygen Windows installer](http://www.doxygen.nl/download.html), version 1.8.15:
 
@@ -154,6 +160,8 @@ scons dist
 
 The build will be created in the dist directory.
 
+### Building the installer
+
 To create a launcher archive (one executable allowing for installation or portable dist generation), type:
 
 ```
@@ -162,6 +170,8 @@ scons launcher
 
 The archive will be placed in the output directory.
 
+### Building the developer documentation
+
 To generate the NVDA developer guide, type:
 
 ```
@@ -179,6 +189,7 @@ scons devDocs_nvdaHelper
 
 The documentation will be placed in the `devDocs\nvdaHelper` folder in the output directory.
 
+### Generate debug symbols archive
 To generate an archive of debug symbols for the various dll/exe binaries, type:
 
 ```
@@ -187,12 +198,14 @@ scons symbolsArchive
 
 The archive will be placed in the output directory.
 
+### Generate translation template
 To generate a gettext translation template (for translators), type:
 
 ```
 scons pot
 ```
 
+### Customising the build
 Optionally, the build can be customised by providing variables on the command line:
 
 * version: The version of this build.
@@ -215,15 +228,15 @@ scons launcher version=test1
 ## Running Automated Tests
 If you make a change to the NVDA code, you should run NVDA's automated tests.
 These tests help to ensure that code changes do not unintentionally break functionality that was previously working.
-Currently, NVDA has two kinds of automated testing: unit tests and translatable string checks.
 
-To run the tests, first change directory to the root of the NVDA source distribution as above.
+To run the tests (unit tests, translatable string checks), first change directory to the root of the NVDA source distribution as above.
 Then, run:
 
 ```
 scons tests
 ```
 
+### Unit tests
 To run only specific unit tests, specify them using the `unitTests` variable on the command line.
 The tests should be provided as a comma separated list.
 Each test should be specified as a Python module, class or method relative to the `tests\unit` directory.
@@ -233,12 +246,25 @@ For example, to run only methods in the `TestMove` and `TestSelection` classes i
 scons tests unitTests=test_cursorManager.TestMove,test_cursorManager.TestSelection
 ```
 
+### Translatable string checks
 To run only the translatable string checks (which check that all translatable strings have translator comments), run:
 
 ```
 scons checkPot
 ```
 
+### Linting your changes
+In order to ensure your changes comply with NVDA's coding style you can run the Flake8 linter locally.
+Running via SCons will use Flake8 to inspect only the differences between your working directory and the specified `base` branch.
+If you create a Pull Request, the `base` branch you use here should be the same as the target you would use for a Pull Request. In most cases it will be `origin/master`.
+```
+scons lint base=origin/master
+```
+
+To be warned about linting errors faster, you may wish to integrate Flake8 other development tools you are using.
+For more details, see `tests/lint/readme.md`
+
+### System Tests
 You may also use scons to run the system tests, though this will still rely on having set up the dependencies (see `tests/system/readme.md`).
 
 ```

sconstruct

@@ -76,6 +76,17 @@ vars.Add(ListVariable("nvdaHelperDebugFlags", "a list of debugging features you
 vars.Add(EnumVariable('nvdaHelperLogLevel','The level of logging you wish to see, lower is more verbose','15',allowed_values=[str(x) for x in range(60)]))
 if "tests" in COMMAND_LINE_TARGETS:
 	vars.Add("unitTests", "A list of unit tests to run", "")
+if "lint" in COMMAND_LINE_TARGETS:
+	vars.Add(  # pass through variable that lint is requested
+		"doLint",
+		"internal use",
+		True
+	)
+	vars.Add(
+		"base",
+		"Lint is done only on a diff, specify the ref to use as base for the diff.",
+		None
+	)
 if "systemTests" in COMMAND_LINE_TARGETS:
 	vars.Add("filter", "A filter for the name of the system test(s) to run. Wildcards accepted.", "")
 

tests/lint/.gitignore

@@ -0,0 +1,2 @@
+current.diff
+current.lint

tests/lint/createJunitReport.py

@@ -0,0 +1,65 @@
+# -*- coding: UTF-8 -*-
+# A part of NonVisual Desktop Access (NVDA)
+# This file is covered by the GNU General Public License.
+# See the file COPYING for more details.
+# Copyright (C) 2019 NV Access Limited
+
+import os
+from sys import argv
+
+NO_ERROR = r'''<?xml version="1.0" encoding="UTF-8"?>
+<testsuite name="flake8" tests="1" errors="0" failures="0" skip="0">
+<testcase classname="flake8.lint" name="flake8_diff_lint" time="1.00">
+</testcase>
+</testsuite>
+'''
+
+# With Error:
+WE_PRE = r'''<?xml version="1.0" encoding="UTF-8"?>
+<testsuite name="flake8" tests="1" errors="1" failures="0" skip="0">
+<testcase classname="flake8.lint" name="flake8_diff_lint" time="1.00">
+<error type="lintError" message="Linting errors occurred">
+<![CDATA[
+'''
+WE_POST = r'''
+]]>
+</error>
+</testcase>
+</testsuite>
+'''
+
+
+def makeJunitXML(inFileName, outFileName):
+	with open(inFileName, 'rt', encoding='UTF-8') as flake8In:
+		errorText = flake8In.read()
+	if len(errorText) > 0:
+		# make "with error" xml content
+		outContents = f'{WE_PRE}{errorText}{WE_POST}'
+	else:
+		# make "no error" xml content
+		outContents = NO_ERROR
+
+	with open(outFileName, 'wt', encoding='UTF-8') as out:
+		out.write(outContents)
+
+
+def main():
+	try:
+		if len(argv) != 3:
+			raise RuntimeError(
+				f"{argv[0]} expects two arguments: flake8_output_file_name junit_file_name"
+			)
+		scriptName, flake8OutputFileName, junitFileName = argv
+		if not os.path.isfile(flake8OutputFileName):
+			raise RuntimeError(
+				f"Flake8_output_file does not exist at {flake8OutputFileName}"
+			)
+		makeJunitXML(flake8OutputFileName, junitFileName)
+	except Exception as e:
+		print(e)
+		raise e
+
+
+if __name__ == "__main__":
+	# execute only if run as a script
+	main()

tests/lint/flake8.ini

@@ -0,0 +1,35 @@
+[flake8]
+
+# Plugins
+use-flake8-tabs = True
+use-pycodestyle-indent = True
+
+# Reporting
+statistics = True
+doctests = True
+show-source = True
+
+# Options
+max-complexity = 15
+max-line-length = 110
+hang-closing = True
+
+ignore =
+	W191, # indentation contains tabs
+	E126, # continuation line over-indented for hanging indent
+	E133, # closing bracket is missing indentation
+	W503, # line break before binary operator. As opposed to W504 (line break after binary operator) which we want to check for.
+
+builtins = # inform flake8 about functions we consider built-in.
+	_, # translation lookup
+	pgettext, # translation lookup
+
+exclude = # don't bother looking in the following subdirectories / files.
+	.git,
+	__pycache__,
+	.tox,
+	build,
+	output,
+	include,
+	miscDeps,
+	source/louis,