Re: [PATCH] tests: add a "check-flake8" test for validating python code style

[John Snow]

On 4/29/20 11:36 AM, Daniel P. Berrangé wrote:
> The flake8 program is a standard tool used by Python projects for
> validating many commonly recommended style rules. It would be desirable
> for QEMU to come into alignment with normal Python coding style best
> practices.
> 
> QEMU currently violates a huge number of the style rules, so we can't
> blindly turn it on. Instead this patch introduces use of flake8 with
> a huge ignore list to turn off everything that is currently violated.
> 
> The following descriptions are mostly taken from:
> 
>   https://www.flake8rules.com/
> 
> Indentation:
> 
>  E111         Indentation is not a multiple of four
>  E114         Indentation is not a multiple of four (comment)
>  E115   Expected an indented block (comment)
>  E116         Unexpected indentation (comment)
>  E117   Over-indented
>  E121   Continuation line under-indented for hanging indent
>  E122         Continuation line missing indentation or outdented
>  E123         Closing bracket does not match indentation of opening bracket's 
> line
>  E124         Closing bracket does not match visual indentation
>  E125         Continuation line with same indent as next logical line
>  E126         Continuation line over-indented for hanging indent
>  E127         Continuation line over-indented for visual indent
>  E128         Continuation line under-indented for visual indent
>  E129         Visually indented line with same indent as next logical line
>  E131         Continuation line unaligned for hanging indent
> 
> Whitespace:
> 
>  E201         Whitespace after '('
>  E202         Whitespace before ')'
>  E203         Whitespace before ':'
>  E211         Whitespace before '('
>  E221         Multiple spaces before operator
>  E222         Multiple spaces after operator
>  E225         Missing whitespace around operator
>  E226         Missing whitespace around arithmetic operator
>  E228         Missing whitespace around modulo operator
>  E231         Missing whitespace after ',', ';', or ':'
>  E241         Multiple spaces after ','
>  E251         Unexpected spaces around keyword / parameter equals
>  E261         At least two spaces before inline comment
>  E265         Block comment should start with '# '
>  E266         Too many leading '#' for block comment
> 
> Blank lines:
> 
>  E301         Expected 1 blank line, found 0
>  E302         Expected 2 blank lines, found 0
>  E303         Too many blank lines (3)
>  E305         Expected 2 blank lines after end of function or class
>  E306         Expected 1 blank line before a nested definition
> 
> Imports:
> 
>  E401         Multiple imports on one line
>  E402         Module level import not at top of file
> 
> Line length:
> 
>  E501         Line too long (82 > 79 characters)
>  E502         The backslash is redundant between brackets
> 
> Statements:
> 
>  E701         Multiple statements on one line (colon)
>  E702         Multiple statements on one line (semicolon)
>  E703         Statement ends with a semicolon
>  E711         Comparison to none should be 'if cond is none:'
>  E712         Comparison to true should be 'if cond is true:' or 'if cond:'
>  E713         Test for membership should be 'not in'
>  E714         Test for object identity should be 'is not'
>  E722   Do not use bare 'except'
>  E731         Do not assign a lambda expression, use a def
>  E741         Do not use variables named 'I', 'O', or 'l'
> 
> Errors:
> 
>  F401         Module imported but unused
>  F403         'from module import *' used; unable to detect undefined names
>  F405         Name may be undefined, or defined from star imports: module
>  F811         Redefinition of unused name from line n
>  F821         Undefined name name
>  F841         Local variable name is assigned to but never used
> 
> Warnings:
> 
>  W391         Blank line at end of file
>  W503         Line break occurred before a binary operator
>  W504         Line break occurred after a binary operator
>  W605         Invalid escape sequence 'x'
> 
> Over time code should be fixed and rules removed from the ignore list.
> A handful of style rules may not warrant fixing as the cure is arguably
> worse and very subjective. e.g.
> 
>  E501: Force breaking lines at < 80 characters results in
>        some really unnatural code formatting which harms
>        readability.
> 
>  W504: Knuth code style requires the operators "or" and "and" etc
>        to be at the start of line in a multi-line conditional.
> 
> Signed-off-by: Daniel P. Berrangé <address@hidden>
> ---
> 
> On its own this patch doesn't really do much of use except try to stop the
> situation getting worse. To be valuable some motivated contributor(s)
> would need to go through fixing the code, and re-enabling each excluded
> warning category one at a time.
> 
> I'm mostly proposing this patch as a starting point for discussion, to
> see if anyone is indeed motivated to take on the code cleanup challenge,
> and feed the fixes in through the various maintainers trees.
> 
>  tests/Makefile.include | 38 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 51de676298..f08e99b09c 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -4,7 +4,7 @@
>  check-help:
>       @echo "Regression testing targets:"
>       @echo
> -     @echo " $(MAKE) check                Run unit, qapi-schema, qtest and 
> decodetree"
> +     @echo " $(MAKE) check                Run unit, qapi-schema, qtest and 
> decodetree flake8"
>       @echo
>       @echo " $(MAKE) check-qtest-TARGET   Run qtest tests for given target"
>       @echo " $(MAKE) check-qtest          Run qtest tests"
> @@ -19,6 +19,7 @@ check-help:
>       @echo " $(MAKE) check-report.tap     Generates an aggregated TAP test 
> report"
>       @echo " $(MAKE) check-venv           Creates a Python venv for tests"
>       @echo " $(MAKE) check-clean          Clean the tests and related data"
> +     @echo " $(MAKE) check-flake8         Clean the tests and related data"
>       @echo
>       @echo " $(MAKE) get-vm-images        Downloads all images used by 
> acceptance tests, according to configured targets (~350 MB each, 1.5 GB max)"
>       @echo
> @@ -923,7 +924,40 @@ check-qtest: $(patsubst %,check-qtest-%, 
> $(QTEST_TARGETS))
>  ifeq ($(CONFIG_TOOLS),y)
>  check-block: $(patsubst %,check-%, $(check-block-y))
>  endif
> -check: check-block check-qapi-schema check-unit check-softfloat check-qtest 
> check-decodetree
> +
> +is_git := $(shell test -d $(SRC_PATH)/.git && echo 1 || echo 0)
> +
> +ifeq (1, $(is_git))
> +PYTHON_FILES = $(shell git ls-tree -r HEAD: --name-only | grep '.py$$')
> +PYTHON_FILES += $(shell git ls-tree -r HEAD: --name-only 
> tests/qemu-iotests/??? | xargs grep -l '/usr/bin/env python')
> +
> +# Validate many python style rules
> +FLAKE8_INDENTATION = 
> E111,E114,E115,E116,E117,E121,E122,E123,E124,E125,E126,E127,E128,E129,E131
> +FLAKE8_WHITESPACE = 
> E201,E202,E203,E211,E221,E222,E225,E226,E228,E231,E241,E251,E261,E265,E266
> +FLAKE8_BLANK_LINES = E301,E302,E303,E305,E306
> +FLAKE8_IMPORTS = E401,E402
> +FLAKE8_LINE_LENGTH = E501,E502
> +FLAKE8_STATEMENTS = E701,E702,E703,E711,E712,E713,E714,E722,E731,E741
> +FLAKE8_ERRORS = F401,F403,F405,F811,F821,F841
> +FLAKE8_WARNINGS = W391,W503,W504,W605
> +
> +FLAKE8_IGNORE = $(FLAKE8_INDENTATION),$\
> +               $(FLAKE8_WHITESPACE),$\
> +               $(FLAKE8_BLANK_LINES),$\
> +               $(FLAKE8_IMPORTS),$\
> +               $(FLAKE8_LINE_LENGTH),$\
> +               $(FLAKE8_STATEMENTS),$\
> +               $(FLAKE8_ERRORS),$\
> +               $(FLAKE8_WARNINGS) \
> +               $(NULL)
> +
> +check-flake8:
> +     $(call quiet-command,flake8 --ignore=$(FLAKE8_IGNORE) $(PYTHON_FILES))
> +else
> +check-flake8:
> +endif
> +
> +check: check-block check-qapi-schema check-unit check-softfloat check-qtest 
> check-decodetree check-flake8
>  check-clean:
>       rm -rf $(check-unit-y) tests/*.o tests/*/*.o $(QEMU_IOTESTS_HELPERS-y)
>       rm -rf $(sort $(foreach target,$(SYSEMU_TARGET_LIST), 
> $(check-qtest-$(target)-y:%=tests/qtest/%$(EXESUF))) 
> $(check-qtest-generic-y:%=tests/qtest/%$(EXESUF)))
> 

Worth pointing out that I have a patchset destined for iotests that
sneaks in a pylintrc file that establishes a much stricter subset for
code in that directory.

It didn't add a makefile entry for it, though.

I also have a lot of patches that work on cleaning up the rest of the
iotest .py files and bringing them up to speed on my pylint subset,
maybe I can take this patch as well and make sure that this minimum is
enforced.

Don't let that stop review on this patch, though. I think it's good to
start enforcing a subset, even if it's almost nothing at all. (And in
practice, I use *both* flake8 and pylint on personal projects, so don't
let that be a criticism against, either.)

--js