qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Markus Armbruster
Subject: Re: [PATCH] tests: add a "check-flake8" test for validating python code style
Date: Thu, 30 Apr 2020 07:23:59 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux)

Daniel P. Berrangé <address@hidden> writes:

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

Ah, a bikeshed!  I really dislike long lines, and I really like Knuth
style ;-P

> 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"

Shouldn't this be "qtest, decodetree, and flake8"?

Would it make sense to subsume flake8 under a general check-style
target?

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

"Validate"?  Those are the style rules we *don't* check...

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

The QAPI generator is already clean except for
F403,F405,E241,W503,W504,E226,E501,E261.  The new automated cleanliness
test is next to useless for keeping it that way.  How could we tailor it
to solve that?




reply via email to

[Prev in Thread] Current Thread [Next in Thread]