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

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

Daniel P. Berrangé <berrange@redhat.com> 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é <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"

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?

On Thu, Apr 30, 2020 at 07:23:59AM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> > +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?

We would have to run flake8 multiple times, passing different exclusions
for different sets of files.  This wouldn't be too bad as long as we don't
get too many different sets of files. We could split it into iotests,
qapi and misc for example.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Thu, Apr 30, 2020 at 07:23:59AM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> > +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?
>
> We would have to run flake8 multiple times, passing different exclusions
> for different sets of files.  This wouldn't be too bad as long as we don't
> get too many different sets of files. We could split it into iotests,
> qapi and misc for example.

Sounds good to me!

On 29/04/20 17:36, 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/

I suggest instead using "black" and just reformat everything in a huge
patch; that's what we're using for Patchew.

It's not perfect, and especially one needs to get used to the double
quotes instead of single quotes for strings.  However overall it is
pretty good, and I've never seen it do something clearly "wrong".

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

If we go with "black" I suggest just doing a big patch for everything,
since it's easy for maintainers to rebase by running black at each step.
 Overall our usage of Python is small enough that it should not be a big
deal.

Paolo

On Thu, Apr 30, 2020 at 12:29:28PM +0200, Paolo Bonzini wrote:
> On 29/04/20 17:36, 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/
> 
> I suggest instead using "black" and just reformat everything in a huge
> patch; that's what we're using for Patchew.

Personally I'm a fan of automated code formatters that can enforce a
specific style, so no objection from me.

> It's not perfect, and especially one needs to get used to the double
> quotes instead of single quotes for strings.  However overall it is
> pretty good, and I've never seen it do something clearly "wrong".

I've not looked at black in any detail, but if its focused on code
style, then there might be a few bits from flake8 that are still
useful, such as checking for unused imports, or unresolved variable
names. Then again, perhaps Jon's pylint stuff will already catch
that.

> > 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.
> 
> If we go with "black" I suggest just doing a big patch for everything,
> since it's easy for maintainers to rebase by running black at each step.
>  Overall our usage of Python is small enough that it should not be a big
> deal.



Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|