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

[John Snow <jsnow@redhat.com>]

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é <berrange@redhat.com>
> ---
> 
> 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