The checks have many issues:
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | davidgumberg, kevkevinpal, achow101 |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
257 .args(get_pathspecs_exclude_subtrees()),
258 )?;
259
260 let mut cmd = Command::new(bin_name);
261- cmd.arg("check").args(checks).args(files.lines());
262+ cmd.args(["check", "--preview"])
I am not sure how I feel about this --preview flag
According to their github docs in the configuration section
To opt in to the latest lint rules, formatter style changes, interface updates, and more, enable preview mode by setting preview = true in your configuration file or passing –preview on the command line. Preview mode enables a collection of unstable features that may change prior to stabilization
Yes, I am aware. However, the dependency is fully pinned, so if a feature changes, it will be noticed. This is no different from any of the other dependencies being pinned to avoid silently breaking updates.
However, it isn’t needed, so I’ve removed it to restore the previous flags.
77@@ -78,28 +78,20 @@
78 'F632,' # use ==/!= to compare str, bytes, and int literals
79 'F701,' # a break statement outside of a while or for loop
80 'F702,' # a continue statement outside of a while or for loop
81- 'F703,' # a continue statement in a finally block in a loop
82 'F704,' # a yield or yield from statement outside of a function
83- 'F705,' # a return statement with arguments inside a generator
84 'F706,' # a return statement outside of a function/method
In commit: lint: Remove python lint rules that are SyntaxError
Nit: I believe all of F701-F707 result in SyntaxError and should be removed
f701.py:
0def func(x):
1 if x > 5:
2 break
3
4for i in range(10):
5 func(i) # <--- call function
f702.py:
0def func(x):
1 if x > 5:
2 continue
3
4for i in range(10):
5 func(i)
f704.py:
0step = 1
1
2for x in range(0, 10, step):
3 for y in range(0, 10, step):
4 yield (x,y)
f706.py:
0step = 1
1
2for x in range(0, 10, step):
3 for y in range(0, 10, step):
4 return (x,y)
f707.py:
0try:
1 x = 1 / 0
2except ZeroDivisionError:
3 print("Divided by zero")
4except:
5 print("Generic exception")
6except ValueError:
7 print("Value error")
61@@ -82,7 +62,6 @@
62 'F706,' # a return statement outside of a function/method
63 'F707,' # an except: block as not the last exception handler
64 'F811,' # redefinition of unused name from line N
65- 'F812,' # list comprehension redefines 'foo' from line N
I fail to see how it could possibly catch a bug, or how this has caught a bug (or ever hit at all) over the last years.
Apart from this, shadowing is allowed in all languages/contexts used by this project.
It seems inconsistent, or mostly a style-taste either way.
49- 'E275,' # missing whitespace after keyword
50- 'E304,' # blank lines found after function decorator
51- 'E306,' # expected 1 blank line before a nested definition
52 'E401,' # multiple imports on one line
53 'E402,' # module level import not at top of file
54- 'E502,' # the backslash is redundant between brackets
--preview argument
As explained in the commit message, the Python language has a strong built-in whitespace requirement already. Obviously, one can define more indentation requirements, but this seems arbitrary at best, without a clear benefit of catching real bugs.
Obviously, anyone is free and encouraged to use a Python formatter of their choice locally. This is unchanged by this pull request.
46- 'E272,' # multiple spaces before keyword
47- 'E273,' # tab after keyword
48- 'E274,' # tab before keyword
49- 'E275,' # missing whitespace after keyword
50- 'E304,' # blank lines found after function decorator
51- 'E306,' # expected 1 blank line before a nested definition
In commit: lint: Remove python whitespace and shadowing lint rules
+1 on dropping checks that result in IndentError
some of the other checks removed here seem reasonable to me e.g. E125, and E275, but not available/hidden behind preview flag in ruff so I’m ~0 on whether or not to remove the rest of them
41- 'E714,' # test for object identity should be "is not"
42- 'E721,' # do not compare types, use "isinstance()"
43- 'E722,' # do not use bare 'except'
44- 'E742,' # do not define classes named "l", "O", or "I"
45- 'E743,' # do not define functions named "l", "O", or "I"
46- 'E901,' # SyntaxError: invalid syntax
215+ "F406", // "from module import *" only allowed at module level
216+ "F407", // an undefined __future__ feature name was imported
217+ "F601", // dictionary key name repeated with different values
218+ "F602", // dictionary key variable name repeated with different values
219+ "F621", // too many expressions in an assignment with star-unpacking
220+ "F622", // two or more starred expressions in an assignment (a, *b, *c = d)
nit:
0# F622: two or more starred expressions in an assignment (a, *b, *c = d)
1a, *b, *c = [1, 2, 3, 4]
results in a syntaxerror in Python 2.7.18
204+ "E714", // test for object identity should be "is not"
205+ "E721", // do not compare types, use "isinstance()"
206+ "E722", // do not use bare 'except'
207+ "E742", // do not define classes named "l", "O", or "I"
208+ "E743", // do not define functions named "l", "O", or "I"
209+ "E902", // TokenError: EOF in multi-line string
nit:
0# E902: TokenError: EOF in multi-line string
1"""
2Unterminated string literal
results in a syntaxerror in Python 2.7.18
236+ "W293", // blank line contains whitespace
237+ "W605", // invalid escape sequence "x"
238+ ]
239+ .iter()
240+ .map(|c| format!("--select={}", c))
241+ .collect::<Vec<_>>();
nit: ruff accepts the checks as comma separated values to –select=
0 let checks = format!(
1 "--select={}",
2 [
3 "B006", // mutable-argument-default
4 "B008", // function-call-in-default-argument
5 "E101", // indentation contains mixed spaces and tabs
6 "E401", // multiple imports on one line
7 "E402", // module level import not at top of file
8 "E701", // multiple statements on one line (colon)
9 "E702", // multiple statements on one line (semicolon)
10 "E703", // statement ends with a semicolon
11 "E711", // comparison to None should be 'if cond is None:'
12 "E714", // test for object identity should be "is not"
13 "E721", // do not compare types, use "isinstance()"
14 "E722", // do not use bare 'except'
15 "E742", // do not define classes named "l", "O", or "I"
16 "E743", // do not define functions named "l", "O", or "I"
17 "E902", // TokenError: EOF in multi-line string
18 "F401", // module imported but unused
19 "F402", // import module from line N shadowed by loop variable
20 "F403", // 'from foo_module import *' used; unable to detect undefined names
21 "F404", // future import(s) name after other statements
22 "F405", // foo_function may be undefined, or defined from star imports: bar_module
23 "F406", // "from module import *" only allowed at module level
24 "F407", // an undefined __future__ feature name was imported
25 "F601", // dictionary key name repeated with different values
26 "F602", // dictionary key variable name repeated with different values
27 "F621", // too many expressions in an assignment with star-unpacking
28 "F622", // two or more starred expressions in an assignment (a, *b, *c = d)
29 "F631", // assertion test is a tuple, which are always True
30 "F632", // use ==/!= to compare str, bytes, and int literals
31 "F701", // a break statement outside of a while or for loop
32 "F702", // a continue statement outside of a while or for loop
33 "F704", // a yield or yield from statement outside of a function
34 "F706", // a return statement outside of a function/method
35 "F707", // an except: block as not the last exception handler
36 "F811", // redefinition of unused name from line N
37 "F821", // undefined name 'Foo'
38 "F822", // undefined name name in __all__
39 "F823", // local variable name … referenced before assignment
40 "F841", // local variable 'foo' is assigned to but never used
41 "W191", // indentation contains tabs
42 "W291", // trailing whitespace
43 "W292", // no newline at end of file
44 "W293", // blank line contains whitespace
45 "W605", // invalid escape sequence "x"
46 ]
47 .join(",")
48 );
Tested and code reviewed ACK https://github.com/bitcoin/bitcoin/pull/30723/commits/fad05a7ae834af043d0cf47aeb6ea8faf2dda979
Looks good, left a few non-blocking nits about other checks that could be dropped since any reasonable version of python (>= 2.7.18) errors when these rules are violated, and stylistic nit about the format of arguments.
Tested with the following python file1 containing some of the disallowed linter violations: https://gist.github.com/davidgumberg/1e2e43c7faeb5f96480142213adfaa3c
And our linter complained. In the end I was unable to trigger F821 and F822, as the linter always complains of F405 instead
Generated initially with an LLM, and then manually inspected/corrected. It also appears that github gist swallows tabs and extraneous whitespace. ↩︎
Any kind of syntax error is already reported, so there is no need to
enumerate all possible types of syntax errors of ancient versions of
Python 2 or 3.
The rules have many issues:
* Most are redundant, because Python already has a built-in
IndentationError, a subclass of SyntaxError, to enforce whitespace.
* They are not enforced consistently anyway, see for examples [1][2]
below.
* They are stylistic rules where the author intentionally formatted the
code to be easier to read. Starting to enforce them now would make the
code harder to read and create frustration in the future.
Fix all issues by removing them.
[1]:
test/functional/feature_cltv.py:63:35: E272 [*] Multiple spaces before keyword
|
61 | # | Script to prepend to scriptSig | nSequence | nLockTime |
62 | # +-------------------------------------------------+------------+--------------+
63 | [[OP_CHECKLOCKTIMEVERIFY], None, None],
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ E272
[2]:
contrib/asmap/asmap.py:395:13: E306 [*] Expected 1 blank line before a nested definition, found 0
|
393 | prefix.pop()
394 | hole = not fill and (lhole or rhole)
395 | def candidate(ctx: Optional[int], res0: Optional[list[ASNEntry]],
| ^^^ E306
Also, change the linter name, needed for the next commit.
Previously they may have taken more than 10 seconds. Now they should
finish in less than one second.
This also allows to drop one dependency to be installed.
review and tested reACK https://github.com/bitcoin/bitcoin/commit/fafdb7df34507eee735893aa871da6ae529e6372
The rebase drops checks for patterns that would result in SyntaxError in modern versions of python and makes a small improvement to the way the lint runner constructs the argument string for ruff.
ACK fafdb7d
Removal of the unnecessary lint checks and the speed up of the checks are great updates!
