Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lint with flake8 #9958

Merged
merged 22 commits into from Aug 1, 2019

Conversation

@feerrenrut
Copy link
Contributor

commented Jul 22, 2019

Link to issue number:

Closes #5918

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:

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)

feerrenrut added some commits Jul 22, 2019

Lint with flake8 on a local diff
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.
Appveyor to run lint
Test result must be converted to junit compatible xml.
Make sure we fetch the base branch first
We do a shallow clone when initially fetching the repo, so we need to get the base branch as well now.
@leonardder
Copy link
Collaborator

left a comment

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

tests/lint/createJunitReport.py Outdated Show resolved Hide resolved
tests/lint/flake8.ini Show resolved Hide resolved
tests/lint/sconscript Outdated Show resolved Hide resolved
@leonardder

This comment has been minimized.

Copy link
Collaborator

commented Jul 24, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

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

This comment has been minimized.

Copy link
Collaborator

commented Jul 24, 2019

@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.

feerrenrut added some commits Jul 24, 2019

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

There might be a better way to do this. It would be nice if arguments
were tied to sub scripts.
appveyor.yml Outdated Show resolved Hide resolved
appveyor.yml Outdated Show resolved Hide resolved

feerrenrut added some commits Jul 25, 2019

Use zero context in unified diff
Errors should not be raised for adjacent code.
@AppVeyorBot

This comment was marked as outdated.

Copy link

commented Jul 25, 2019

 - There are lint errors
@AppVeyorBot

This comment was marked as outdated.

Copy link

commented Jul 25, 2019

Build failed (commit 40bf5df by @feerrenrut)

@AppVeyorBot

This comment was marked as outdated.

Copy link

commented Jul 25, 2019

 :x: [Build Failed](https://ci.appveyor.com/project/NVAccess/nvda/builds/26237106) (commit https://github.com/nvaccess/nvda/commit/7a97bf1143 by @feerrenrut)

 Flake8 errors found in diff.

 See [Test Results](https://ci.appveyor.com/project/NVAccess/nvda/builds/26237106\tests)
@AppVeyorBot

This comment was marked as outdated.

Copy link

commented Jul 25, 2019

Build Failed (commit 7a97bf1 by @feerrenrut)

Flake8 errors found in diff.

See Test Results

@AppVeyorBot

This comment has been minimized.

Copy link

commented Jul 25, 2019

PR introduces Flake8 errors 😲

See test results for Failed build of commit 02b3346201

tests/lint/sconscript Outdated Show resolved Hide resolved
tests/lint/sconscript Outdated Show resolved Hide resolved

feerrenrut and others added some commits Jul 25, 2019

Spacing in missing base argument error message
Co-Authored-By: Leonard de Ruijter <leonardder@users.noreply.github.com>
Spacing in missing base argument error message
Co-Authored-By: Leonard de Ruijter <leonardder@users.noreply.github.com>
@AppVeyorBot

This comment has been minimized.

Copy link

commented Jul 25, 2019

PR introduces Flake8 errors 😲

See test results for Failed build of commit 0def0c5900

@leonardder

This comment has been minimized.

Copy link
Collaborator

commented Jul 29, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

Are you using the config file from this PR?

@leonardder

This comment has been minimized.

Copy link
Collaborator

commented Jul 29, 2019

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

@AppVeyorBot

This comment has been minimized.

Copy link

commented Jul 29, 2019

PR introduces Flake8 errors 😲

See test results for Failed build of commit dfe787d60b

@AppVeyorBot

This comment has been minimized.

Copy link

commented Jul 30, 2019

PR introduces Flake8 errors 😲

See test results for Failed build of commit 931f1598d7

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

@feerrenrut feerrenrut marked this pull request as ready for review Jul 30, 2019

@feerrenrut feerrenrut requested a review from leonardder Jul 30, 2019

Ignore Flake8 warning: W503
W503 is: line break before binary operator.
Contrasted with W504: line break after binary operator, which we want to check for.
@leonardder
Copy link
Collaborator

left a comment

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

# 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

This comment has been minimized.

Copy link
@leonardder

leonardder Jul 31, 2019

Collaborator

Could you add an empty line above this line?

tests/lint/createJunitReport.py Show resolved Hide resolved
appveyor.yml Show resolved Hide resolved
tests/lint/flake8.ini Show resolved Hide resolved
tests/lint/genDiff.py Outdated Show resolved Hide resolved
tests/lint/genDiff.py Show resolved Hide resolved
tests/lint/lintInstall/requirements.txt Outdated Show resolved Hide resolved
tests/lint/sconscript Show resolved Hide resolved
tests/lint/sconscript Show resolved Hide resolved

feerrenrut and others added some commits Aug 1, 2019

Update Flake8 version
Allow automatic changes to minor versions.

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

This comment has been minimized.

Copy link
Collaborator

commented Aug 1, 2019

Note that the base of this is still threshold.

@feerrenrut

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

Note that the base of this is still threshold.

Thanks 👍

@feerrenrut feerrenrut changed the base branch from threshold to master Aug 1, 2019

@feerrenrut feerrenrut merged commit e68ce2d into master Aug 1, 2019

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@nvaccessAuto nvaccessAuto added this to the 2019.3 milestone Aug 1, 2019

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

@josephsl

This comment has been minimized.

Copy link
Collaborator

commented Aug 2, 2019

Hi,

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

  1. Some text editors may not expose indentation formatting correctly, leading some to believe the code has been indented when it didn't.
  2. It produces E101 (mixture of tabs and spaces). For long lines, it can cause line length issue to pop up.
  3. 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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

@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

This comment has been minimized.

Copy link
Collaborator

commented Aug 2, 2019

@bedenhofer bedenhofer referenced this pull request Aug 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.