Re-enable pylint CI checker #5096
Conversation
| @@ -18,6 +18,7 @@ jobs: | |||
| - run: sudo apt update | |||
| # TODO: update checkers to current versions available in ubuntu 22.04 | |||
| # - run: sudo apt install doxygen clang-format-10 cppcheck pylint python-serial | |||
| - run: sudo apt install pylint | |||
There was a problem hiding this comment.
I would keep the rest in the comment. I suspect they will be needed.
There was a problem hiding this comment.
I was thinking they would the added back as needed, also some of their names are different in ubuntu-22.04.
There was a problem hiding this comment.
The comment shows the missing tools, regardless of the new name. If we delete it, we don't know which tools are still not added back. Instead, we should delete them one by one when they are added back.
tools/pylint/pylintrc
Outdated
| @@ -59,7 +59,7 @@ confidence= | |||
| # --enable=similarities". If you want to run only the classes checker, but have | |||
| # no Warning level messages displayed, use"--disable=all --enable=classes | |||
| # --disable=W" | |||
| disable=import-star-module-level,old-octal-literal,oct-method,unpacking-in-except,parameter-unpacking,backtick,old-raise-syntax,old-ne-operator,long-suffix,dict-view-method,dict-iter-method,metaclass-assignment,next-method-called,raising-string,indexing-exception,raw_input-builtin,long-builtin,file-builtin,execfile-builtin,coerce-builtin,cmp-builtin,buffer-builtin,basestring-builtin,apply-builtin,filter-builtin-not-iterating,using-cmp-argument,useless-suppression,range-builtin-not-iterating,suppressed-message,no-absolute-import,old-division,cmp-method,reload-builtin,zip-builtin-not-iterating,intern-builtin,unichr-builtin,reduce-builtin,standarderror-builtin,unicode-builtin,xrange-builtin,coerce-method,delslice-method,getslice-method,setslice-method,input-builtin,round-builtin,hex-method,nonzero-method,map-builtin-not-iterating,missing-docstring,locally-disabled | |||
| disable=import-star-module-level,old-octal-literal,oct-method,unpacking-in-except,parameter-unpacking,backtick,old-raise-syntax,old-ne-operator,long-suffix,dict-view-method,dict-iter-method,metaclass-assignment,next-method-called,raising-string,indexing-exception,raw_input-builtin,long-builtin,file-builtin,execfile-builtin,coerce-builtin,cmp-builtin,buffer-builtin,basestring-builtin,apply-builtin,filter-builtin-not-iterating,using-cmp-argument,useless-suppression,range-builtin-not-iterating,suppressed-message,no-absolute-import,old-division,cmp-method,reload-builtin,zip-builtin-not-iterating,intern-builtin,unichr-builtin,reduce-builtin,standarderror-builtin,unicode-builtin,xrange-builtin,coerce-method,delslice-method,getslice-method,setslice-method,input-builtin,round-builtin,hex-method,nonzero-method,map-builtin-not-iterating,missing-docstring,locally-disabled,consider-using-f-string,unspecified-encoding,consider-using-from-import,consider-using-with,consider-using-in,no-else-return,no-else-continue,no-else-break,consider-using-sys-exit,chained-comparison,useless-object-inheritance,no-else-raise,singleton-comparison,redundant-u-string-prefix,import-outside-toplevel | |||
There was a problem hiding this comment.
Yes, the following ones are new additions:
- consider-using-f-string
- unspecified-encoding
- consider-using-from-import
- consider-using-with
- consider-using-in
- no-else-return
- no-else-continue
- no-else-break
- consider-using-sys-exit
- chained-comparison
- useless-object-inheritance
- no-else-raise
- singleton-comparison
- redundant-u-string-prefix
- import-outside-toplevel
There was a problem hiding this comment.
How much these affect our code? Linting looses its purpose if we disable everything. If there are not much changes, we could enable them.
There was a problem hiding this comment.
The big one is consider-using-f-string, that affects most places strings are formatted, the rest only affect a couple lines each.
There was a problem hiding this comment.
Do we have an exact minimum requirement for python version? f-strings have "only" been supported since py36. It may not be a big deal if we consider that officially even py37 is EOL, but some developers my be stuck on older systems where old python versions may only be available. (As I recall, that used to be the reason for using py2 for a very long time.)
There was a problem hiding this comment.
I do not think we have a set minimum, and for now I left consider-using-f-string and 2 others in specific places disabled, and fixed the rest.
tools/pylint/pylintrc
Outdated
| @@ -297,7 +297,7 @@ ignore-comments=yes | |||
| ignore-docstrings=yes | |||
|
|
|||
| # Ignore imports when computing similarities. | |||
| ignore-imports=no | |||
| ignore-imports=yes | |||
There was a problem hiding this comment.
What is the effect of this change?
There was a problem hiding this comment.
In the newer version of pylint the thresholds on duplicate code are different, and in two files the imports are flagged as duplicate code
as far as I know there is now way to disable the duplicate-code checker for only the relevant sections of the files, and I did not want to disable checking for duplicate code entirely
************* Module jerry_client_main
jerry-debugger/jerry_client_main.py:1:0: R0801: Similar lines in 2 files
==run-test-suite:[19:26]
==run-unittests:[20:28]
import os
import subprocess
import sys
import util
def get_arguments(): (duplicate-code)
There was a problem hiding this comment.
Maybe we could rewrite the code to avoid duplicates, if it is not too ugly.
There was a problem hiding this comment.
Renaming the get_arguments function in one of the files is enough.
matetokodi
force-pushed
the
update_pylint
branch
2 times, most recently
from
8474bbe
to
68cf4d9
Compare
matetokodi
force-pushed
the
update_pylint
branch
3 times, most recently
from
3190607
to
65045cb
Compare
|
Over time this became an overall Python modernization patch, with the following additions:
|
matetokodi
force-pushed
the
update_pylint
branch
from
65045cb
to
b902669
Compare
Update code to conform to the newer version of pylint available in
ubuntu-22.04, with few exceptions:
- disabled `import-outside-toplevel` for `main()` in
`jerry_client.py`
- disabled `consider-using-with` for the logfile of `TestSuite` in
`test262-harness.py` as using `with` is not practical in that case
Update test262-harness.py to use argparse instead of the now deprecated
optparse
Rename variables in jerry_client_main.py that redefined python builtins
or shadowed variables from an outer scope
Update python files to use f-stirngs
Add minimum python versions (3.6 and 3.8) to the CI jobs: without it the
default python version did not support the `with` statement for
`subprocess.Popen` used in `build.py` on macos, or in some cases f-stirngs
Remove `from __future__` imports that are no-ops in python 3
Remove shebang from non executable files
Re-enable most pylint checkers, except `missing-docstring`
JerryScript-DCO-1.0-Signed-off-by: Máté Tokodi [email protected]
matetokodi
force-pushed
the
update_pylint
branch
from
b902669
to
884a1a7
Compare
This patch re-enables the pylint CI checker
Adjust pylintrc to account for new checkers introduced since updating from ubuntu-18.04 to ubuntu-22-04: ignore imports, disable some style-based checks: e.g: consider-using-f-string, no-else-continue, among others.
Update
test262-harness.pyto useargparseinstead of the now deprecatedoptparse.Rename variables in
jerry_client_main.pythat redefined python builtins or shadowed variables from an outer scope