Lint with flake8 by feerrenrut · Pull Request #9958 · nvaccess/nvda

feerrenrut · 2019-07-22T19:56:32Z

Link to issue number:

Summary of the issue:

Code contributors regularly have to deal with ill-defined and inconsistently enforced code style requirements. Code reviewers spend much of their time reporting minor issues, time that would be better spent looking for architectural problems / product issues / logic errors.

Description of how this pull request fixes the issue:

Automate checking python style. The diff from new PR's will be tested for compliance with Flake8. The NVDA Python code already contains several inconsistent styles, so rather than try to match it I have tried to configure Flake8 to use the default style guidelines as much as possible.

This pr introduces two new SCons build targets:

lint
- creates a unified diff with git diff -U0 $(git merge-base <baseBranch>)
  - A helper script is used to generate this diff (tests\lint\genDiff.py)
- The diff is piped to flake8 to perform the linting.
- The output is printed to stdout and also to tests/lint/current.lint
lintInstall
- required by lint.
- Uses pip to install dependencies from a requirements.txt file.

AppVeyor:

Adds a new script for tests phase of build
Mostly does what SCons does, does not need to worry about getting working tree / uncommit changes into the diff.
In order to preserve the availability of artifacts, these are uploaded from a on_finish phase rather than artifacts phase.
- This acts like a "finally" block, and happens regardless of whether the build passes or fails.
- The installer artifact is often used to test if a change fixes an issue before the PR is polished off / reviewed. It also can help reviewers to test a change locally without having to build the branch.
A message is sent when there are linting errors.
A failed lint will still halt the build, system tests are not run.

Testing performed:

Tested locally:

Enured that uncommited and unstaged changes in the working tree are included in diff given to Flake8
Checked that #noqa: <code> comments work to suppress errors.

Tested on appveyor:

Sample output for build with errors: Lint with flake8 #9958 (comment)
Sample output for clean build: Search for flake8.lint.flake8_diff_lint on https://ci.appveyor.com/project/NVAccess/nvda/builds/26350748/tests

Known issues with pull request:

Add documentation to readme.
Test file to demonstrate, needs removal.
Using --disable-noqa to demonstrate test file, needs removal
Alters the users installed packages, see lintInstall
Requires git in path.

Change log entry:

Changes for developers:
- Flake8 linting tool has been integrated with SCons reflecting code requirements for Pull Requests. (#5918)

Uses flake8-tabs so that we can use tab rather than spaces. Includes a default config file for flake8 (flake8.ini) Scons build targets added: lint: Produces a unified diff (against HEAD), and runs flake8 linter against this diff. lintInstall: Uses pip (and a requirements file) to install flake8 dependency. A file test.py is included as a demonstration, this will be removed prior to merging.

Test result must be converted to junit compatible xml.

We do a shallow clone when initially fetching the repo, so we need to get the base branch as well now.

LeonarddeR

I'm aware of this still being a draft, but I'm pretty interested to see this finished.

LeonarddeR · 2019-07-24T11:31:14Z

I have the following suggestions for the configuration of flake8:

Add --count parameter to see number of errors
add miscDeps and source\louis to exclude
Add doctests = True

feerrenrut · 2019-07-24T17:31:23Z

Add --count parameter to see number of errors

count is not so helpful, we already print statistics (giving the number of each error that occurred), count prints an extra number. See the last "1" in the following example output:

tests/lint/createJunitReport.py:44:1: E302 expected 2 blank lines, found 1
def main():
^
1     E302 expected 2 blank lines, found 1
1

LeonarddeR · 2019-07-24T18:00:27Z

@feerrenrut commented on 24 Jul 2019, 19:31 CEST:

Add --count parameter to see number of errors

count is not so helpful, we already print statistics (giving the number of each error that occurred), count prints an extra number. See the last "1" in the following example output:
tests/lint/createJunitReport.py:44:1: E302 expected 2 blank lines, found 1
def main():
^
1     E302 expected 2 blank lines, found 1
1

Ah, I had hoped that it was more descriptive. Never mind.

…se argument. There might be a better way to do this. It would be nice if arguments were tied to sub scripts.

Errors should not be raised for adjacent code.

Also send a message when linting fails.

AppVeyorBot · 2019-07-25T14:09:25Z

PR introduces Flake8 errors 😲

See test results for Failed build of commit 02b3346201

Co-Authored-By: Leonard de Ruijter <leonardder@users.noreply.github.com>

AppVeyorBot · 2019-07-25T20:42:39Z

PR introduces Flake8 errors 😲

See test results for Failed build of commit 0def0c5900

LeonarddeR · 2019-07-29T10:27:34Z

Testing with flake8 within VS Code reveals that it raises warnings for using _ and pgettext. I'm afraid this will also be the case for appveyor builds. It is likelike that scons has sourceEnv imported before it uses flake8

feerrenrut · 2019-07-29T11:09:39Z

Are you using the config file from this PR?

LeonarddeR · 2019-07-29T11:44:50Z

Ah I'm sorr, turns out that I was using an out of date version of the branch for my testing purposes.

AppVeyorBot · 2019-07-29T17:09:47Z

PR introduces Flake8 errors 😲

See test results for Failed build of commit dfe787d60b

AppVeyorBot · 2019-07-30T18:18:54Z

PR introduces Flake8 errors 😲

See test results for Failed build of commit 931f1598d7

Remove --disable-noqa Remove test.py full of # noqa: comments

W503 is: line break before binary operator. Contrasted with W504: line break after binary operator, which we want to check for.

LeonarddeR

I think it would be good to remove source/pylintrc while at it. According to @bramd, it has its issues anyway.

LeonarddeR · 2019-07-31T11:40:50Z

+# 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


Could you add an empty line above this line?

Allow automatic changes to minor versions. Co-Authored-By: Leonard de Ruijter <leonardder@users.noreply.github.com>

LeonarddeR · 2019-08-01T14:35:21Z

Note that the base of this is still threshold.

feerrenrut · 2019-08-01T14:57:27Z

Note that the base of this is still threshold.

Thanks 👍

josephsl · 2019-08-02T15:04:52Z

Hi,

Coming back to this, I think it would be helpful to exclude checks for visual indents; although useful, it has drawbacks:

Some text editors may not expose indentation formatting correctly, leading some to believe the code has been indented when it didn't.
It produces E101 (mixture of tabs and spaces). For long lines, it can cause line length issue to pop up.
For long conditional statements, splitting it into multiple lines may introduce more errors than intended, and without wrapping them inside parentheses, syntax error (E999) is raised.

I'll create a new issue to discuss these further.

Thanks.

feerrenrut · 2019-08-02T16:51:40Z

@josephsl we have a few issues to work through with the indentation checks, but I would really like it if we can find an acceptable configuration. This will likely require people to think about the configuration of their tools.

josephsl · 2019-08-02T16:54:32Z

Hi, right. Personally I can deal with indents quite easily thanks to Notepad++, but I imagine others may run into issues (older versions of Notepad, for example). Thanks.

michaelDCurran · 2019-12-10T00:12:07Z

This pr breaks publishing of NVDA releases on our server. I was able to work around this for beta1, but this certainly needs to be addressed for the final release.
In appveyor.yaml, commands in deploy_script make use of the $artifacts variable, which is set by appveyor based on pushed artifacts. However, pushing of artifacts in the output directory, including the NVDA launcher executable, had been moved to the on_finish section, which is run after deploy_script, thus $artifacts does not contain them. Uploading of the output directory really needs to be done probably at the end of build_script.

feerrenrut added 5 commits July 22, 2019 21:28

Appveyor to run lint

1030976

Test result must be converted to junit compatible xml.

Actually do the diff

16e414b

Make sure we fetch the base branch first

8064145

We do a shallow clone when initially fetching the repo, so we need to get the base branch as well now.

Hopefully get the encoding right this time.

8f227a0

LeonarddeR reviewed Jul 23, 2019

View reviewed changes

Comment thread tests/lint/createJunitReport.py Outdated

Comment thread tests/lint/flake8.ini

Comment thread tests/lint/sconscript Outdated

Fix some issues with script and config

17e3fe6

feerrenrut added 3 commits July 24, 2019 23:10

Allow specifying base ref for scons based lint

cbab656

Fix malformed command line for flake8

deb849c

When lint is not provided, we still get an error about the missing ba…

be5af65

…se argument. There might be a better way to do this. It would be nice if arguments were tied to sub scripts.

LeonarddeR reviewed Jul 25, 2019

View reviewed changes

Comment thread appveyor.yml Outdated

LeonarddeR reviewed Jul 25, 2019

View reviewed changes

Comment thread appveyor.yml Outdated

LeonarddeR mentioned this pull request Jul 25, 2019

Provide VS Code support within NVDA"s git repository #9971

Closed

feerrenrut added 2 commits July 25, 2019 12:24

Use zero context in unified diff

4efe661

Errors should not be raised for adjacent code.

Try to ensure that build artifacts are available even on lint failure.

18c0cd6

Also send a message when linting fails.

This comment has been minimized.

Sign in to view

Include stats in lint message

d3a7ca1

This comment has been minimized.

Sign in to view

Leave stats out for now.

71f4bf4

This comment has been minimized.

Sign in to view

Clarify flake8 error message

99449a7

LeonarddeR reviewed Jul 25, 2019

View reviewed changes

Comment thread tests/lint/sconscript Outdated

Comment thread tests/lint/sconscript Outdated

feerrenrut and others added 2 commits July 25, 2019 22:30

Spacing in missing base argument error message

1b4c40e

Co-Authored-By: Leonard de Ruijter <leonardder@users.noreply.github.com>

Spacing in missing base argument error message

f7bee4e

Co-Authored-By: Leonard de Ruijter <leonardder@users.noreply.github.com>

update readme to include new scons targets

292d070

Generate diff including working tree

ac098c3

Remove example

0637ab5

Remove --disable-noqa Remove test.py full of # noqa: comments

feerrenrut marked this pull request as ready for review July 30, 2019 18:44

feerrenrut requested a review from LeonarddeR July 30, 2019 18:54

Ignore Flake8 warning: W503

a4d3b94

W503 is: line break before binary operator. Contrasted with W504: line break after binary operator, which we want to check for.

LeonarddeR reviewed Jul 31, 2019

View reviewed changes

feerrenrut and others added 2 commits August 1, 2019 15:30

Update Flake8 version

2d0869f

Allow automatic changes to minor versions. Co-Authored-By: Leonard de Ruijter <leonardder@users.noreply.github.com>

Review actions

3da1fe5

LeonarddeR approved these changes Aug 1, 2019

View reviewed changes

feerrenrut changed the base branch from threshold to master August 1, 2019 14:57

feerrenrut merged commit e68ce2d into master Aug 1, 2019

nvaccessAuto added this to the 2019.3 milestone Aug 1, 2019

feerrenrut added a commit that referenced this pull request Aug 1, 2019

Update changes file for PR #9958

83d7233

josephsl mentioned this pull request Aug 2, 2019

Flake8/lint: do not check for style errors that conflict with overall objectives of this project #10021

Closed

bedenhofer mentioned this pull request Aug 10, 2019

NVDA remote #10068

Closed

feerrenrut deleted the lint-withFlake8 branch January 17, 2020 08:57

Uh oh!

Conversation

feerrenrut commented Jul 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing performed:

Known issues with pull request:

Change log entry:

Uh oh!

LeonarddeR left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LeonarddeR commented Jul 24, 2019

Uh oh!

feerrenrut commented Jul 24, 2019

Uh oh!

LeonarddeR commented Jul 24, 2019

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

AppVeyorBot commented Jul 25, 2019

Uh oh!

Uh oh!

Uh oh!

AppVeyorBot commented Jul 25, 2019

Uh oh!

LeonarddeR commented Jul 29, 2019

Uh oh!

feerrenrut commented Jul 29, 2019

Uh oh!

LeonarddeR commented Jul 29, 2019

Uh oh!

AppVeyorBot commented Jul 29, 2019

Uh oh!

AppVeyorBot commented Jul 30, 2019

Uh oh!

LeonarddeR left a comment

Choose a reason for hiding this comment

Uh oh!

LeonarddeR Jul 31, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LeonarddeR commented Aug 1, 2019

Uh oh!

feerrenrut commented Aug 1, 2019

Uh oh!

josephsl commented Aug 2, 2019

Uh oh!

feerrenrut commented Aug 2, 2019

Uh oh!

josephsl commented Aug 2, 2019 via email • edited by feerrenrut Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelDCurran commented Dec 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

feerrenrut commented Jul 22, 2019 •

edited

Loading

josephsl commented Aug 2, 2019 via email •

edited by feerrenrut

Loading