[PEP08] Project PEPin!

[guedou]

Goals

During yesterday Scapy meetup, we discussed how we can improve the current state of Scapy source style with the following constraints:

• limit the effects on git history
• perform incremental changes
• ease reviewing
• aim to fix a single type of error per pull-request, and explain why some are not fixed
• enforce PEP08 checks
• credit original authors

Methodology

We performed a flake8 run on commit f89055dac106f79f8892a33eca2e8b0ac29475ff that shows 21733 errors made of 55 different errors/warnings. Scapy LoC is currently 38k.

We decided to classify the errors type into three categories:

1. immediate fixes: errors that not related to the style and can be fixed by regular commits by anyone;
2. to be discussed: these errors need to be fixed carefully. Applying a linter might lead to bugs or hard to review changes;
3. style related: we wish to do a single commit per error. This commit will fix all lines concerned by this error, and will be authored by 'Phil phil@secdev.org'. The error message will take advantage of the 'Co-authored-by' to credit original authors. For example, the following message will be used for E231:

   E231 - missing whitespace after ','
   
   Co-authored-by: gpotter2 <10530980+gpotter2@users.noreply.github.com>
   Co-authored-by: Guillaume Valadon <guillaume@valadon.net
   Co-authored-by: Philippe ROSE <shadawan@gmail.com>
   Co-authored-by: Francois Contat <francois.contat@ssi.gouv.fr>
   [..]
   

Automatic tests will be added and enforced as errors are fixed. In the mean time, the following command can be used to check for already fixed errors:
tox -e flake8

Stats

Here are the errors that need to be fixed. Comments, help and feedback are welcome!

• immediate fixes

%	Count	Type & example
0.234666	51	F821 undefined name 'long'
0.138039	30	F601 dictionary key 'ldapUrl' repeated with different values
0.10583	23	F841 local variable 'name' is assigned to but never used
0.0782221	17	F811 redefinition of unused 'PcapTimeoutElapsed' from line 68
0.0046013	1	F402 import 'conf' from line 30 shadowed by loop variable

• style related

%	Count	Type & example
18.1521	3945	E231 missing whitespace after ','
12.4971	2716	E501 line too long (80 > 79 characters)
9.53849	2073	E302 expected 2 blank lines, found 1
7.40349	1609	E201 whitespace after '{'
6.57065	1428	E203 whitespace before ';'
5.28229	1148	E128 continuation line under-indented for visual indent
5.19947	1130	E202 whitespace before '}'
4.85897	1056	E266 too many leading '#' for block comment
4.81296	1046	E301 expected 1 blank line, found 0
2.72397	592	E225 missing whitespace around operator
2.4709	537	E251 unexpected spaces around keyword / parameter equals
2.45709	534	W293 blank line contains whitespace
1.89113	411	E261 at least two spaces before inline comment
1.73929	378	E111 indentation is not a multiple of four
1.73929	378	E265 block comment should start with '# '
1.72549	375	W291 trailing whitespace
0.980076	213	E305 expected 2 blank lines after class or function definition, found 1
0.92026	200	E127 continuation line over-indented for visual indent
0.81443	177	E303 too many blank lines (2)
0.561358	122	E221 multiple spaces before operator
0.478535	104	E262 inline comment should start with '# '
0.414117	90	E227 missing whitespace around bitwise or shift operator
0.39111	85	E124 closing bracket does not match visual indentation
0.28528	62	W391 blank line at end of file
0.276078	60	E131 continuation line unaligned for hanging indent
0.271477	59	E228 missing whitespace around modulo operator
0.266875	58	W191 indentation contains tabs
0.230065	50	E114 indentation is not a multiple of four (comment)
0.207058	45	E129 visually indented line with same indent as next logical line
0.174849	38	E401 multiple imports on one line
0.174849	38	E701 multiple statements on one line (colon)
0.133438	29	E115 expected an indented block (comment)
0.128836	28	E122 continuation line missing indentation or outdented
0.119634	26	E711 comparison to None should be 'if cond is None:'
0.115032	25	E306 expected 1 blank line before a nested definition, found 0
0.115032	25	E702 multiple statements on one line (semicolon)
0.0966272	21	E222 multiple spaces after operator
0.092026	20	E713 test for membership should be 'not in'
0.0874247	19	E116 unexpected indentation (comment)
0.0736208	16	E271 multiple spaces after keyword
0.0322091	7	E272 multiple spaces before keyword
0.0322091	7	E703 statement ends with a semicolon
0.0092026	2	E101 indentation contains mixed spaces and tabs
0.0092026	2	E211 whitespace before '('
0.0046013	1	E712 comparison to True should be 'if cond is True:' or 'if cond:'

• to be discussed

%	Count	Type & example
1.70708	371	E741 ambiguous variable name 'l'
0.634979	138	E722 do not use bare except
0.363503	79	E402 module level import not at top of file
0.138039	30	E731 do not assign a lambda expression, use a def
0.0368104	8	E502 the backslash is redundant between brackets

[guedou]

Invoking @p-l- and @gpotter2

There are currently three remaining errors:

• 366 - E741 ambiguous variable name 'l'
• 70 - E402 module level import not at top of file
• 33 - E731 do not assign a lambda expression, use a def

I don't want to apply E731. I like assigning lambda to variable when it improve visibility. We could disable this error for the project

E741 will be a nightmare to fix, I suggest that we use noqa comments.

Concerning E402, I have mixed feelings between writing "except Exception" and using noqa comments.

What are your opinions?

[gpotter2]

My opinion about each:

• I agree that E731 is annoying, it's really handy in many ways, when simplifying duplicated code.
• Is E402 really fixable ? In many places, to avoid an import loop, we have to import another scapy module only when running a function
• I would prefer to fix E741 correctly, rather than using noqa. It might take time though :/. Especially on Windows, it's really hard to distinguish 1 and l when using default fonts, which had me tricked several times

[guedou]

I submitted PRs for E402 and E731. We are almost there =)

I will try to fix the single l variables while adding noqa for E741.

What is your call for E722?

[guedou]

With all the PR applied, a run of flake8 v3.5.0 returns the following errors:

Count	Type & example
184	F401 - 'scapy.error.warning' imported but unused
285	F403 - 'from scapy.fields import *' used; unable to detect undefined names
6981	F405 - 'bytes_hex' may be undefined, or defined from star imports: scapy.compat

[guedou]

PR #1626 contains a commit that enable all PE08 checks.