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

Jump to bottom

scons lint #5918

Closed
derekriemer opened this issue Apr 30, 2016 · 4 comments · Fixed by #9958
Closed

scons lint #5918

derekriemer opened this issue Apr 30, 2016 · 4 comments · Fixed by #9958
Milestone
2019.3

Comments

@derekriemer
Copy link
Collaborator

I propose a linter to be built into NVDA's scons fscontruct file. Here are reasons I feel this way.

  1. Code reviews already take a while to complete.
  2. Once complete, the code review can be held up by contributers having to fix simple things like "x=4" to "x = 4"
  3. A whole step could be taken from the equation, and NVDA's source code could be much cleaner indeed if a linter were included in NVDA's development packages.
  4. Adding a note to the contributers guide, telling people to simply run scons lint before they make a PR, because code reviews that don't pass scons lint won't be excepted.

For background knowledge, a linter is a program that does static analysis of code, to ensure quality by forcing a certain structure. I believe linters can either fix the errors for you, or can give an error like
"c:\users\derek\nvda\source\browseMode.py 55:3,
Illegal use of spaces around equals signs, ensure a space is padding both sides of an equals sign instead"
This both improves the quality of code, and helps to ensure that code reviews take less time.

References:

PyLint
[What are the comprehensive lint checkers for python | stack overflow](http://stackoverflow.com/questions/5611776/what-are-the-comprehensive-lint-che
ckers-for-python)
@ctoth
Copy link
Contributor

ctoth commented Aug 19, 2016

I agree this is a fantastic idea.
I've had good experiences with Flake8 https://pypi.python.org/pypi/flake8
You'd need to ship a default config that didn't complain about some of the most-common pep8 violations in NVDA source though.

@leonardder
Copy link
Collaborator

@bramd: You might be interested in this issue.

@bramd
Copy link
Contributor

bramd commented Mar 25, 2018

Good idea. However, to get this right we might need to define the wanted code style first. Current NVDA source can be quite inconsistent. I don't do NVDA work that often and do other Python development as well. For other projects I've to comply with pep8, where NVDA deviates from that on various points, including indentation and name casing. So, a linter would help me a lot to follow NVDA's code style. I think this might also be the same for other Python devs that might contribute code from time to time. When using an editor with integrated linting support, the inline warning/errors really help me to get the code style right.

@Adriani90
Copy link
Collaborator

cc: @michaelDCurran, @feerrenrut

@feerrenrut feerrenrut mentioned this issue Jul 22, 2019
Merged
3 tasks
@nvaccessAuto nvaccessAuto added this to the 2019.3 milestone Aug 1, 2019
feerrenrut added a commit that referenced this issue Aug 1, 2019
@feerrenrut
Integrate Flake8 linting with SCons (PR #9958)
e68ce2d 
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.

In this commit we introduce automated checking of 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.

Added 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 changes:
- 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.

Closes #5918
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2019.3
Development

Successfully merging a pull request may close this issue.

Lint with flake8 nvaccess/nvda
6 participants
@bramd @ctoth @leonardder @derekriemer @nvaccessAuto @Adriani90