Static analysis for Python code (ie: ruff)

Developers
1

As far as I can tell, there isn’t any configuration for a static analysis tool for the python code. Tools like ruff detect a lot of issues in klippy/, some of them irrelevant and some potentially dangerous.

I’m willing to be the main driving force and contributor, following a plan similar to this:

  1. Derive a list of unique issue codes when running ruff with the default config
  2. Decide which warnings/errors are worthwhile and which are irrelevant
  3. Open a PR for each worthwhile warning/error
  4. Derive a list of unique issue codes when running ruff with --include ALL
  5. repeat step 2
  6. repeat step 3
  7. write a ruff.toml which ignores the irrelevant rules discussed in steps 1 and 4
  8. integrate ruff as a CI step/github action/pre-commit hook
2 Likes
2

I’m happy to assist in this as well.

3

Thanks. I’m not familiar with that tool. I guess the main question I have is - what kinds of errors does it find on the current code?

If it can reliably find “real” errors than I think it could have value adding it to Klipper’s regression test suite. (By “errors” I mean catching real-world faults that could occur during run-time.) We’d then need to evaluate the balance between false-positives and actual found errors.

However, if it reports on indentation or “style issues” than I’m not, personally, interested in it. As, in my experience, chasing a particular coding style leads to unnecessary code churn that complicates merges and can introduce subtle errors.

Cheers,
-Kevin

2 Likes
4

I’ve opened a PR for the roff.toml config file at [WIP] ruff: init by matdibu · Pull Request #6651 · Klipper3d/klipper · GitHub

5

If I understood Kevin correctly, the real question is, whether the majority of identified issues are “stylistic noise” or real errors that would need attention.

This is the current code base and around 30% are “Multiple imports on one line”.
What are real issues and what might just be “sloppiness” or unneeded is beyond me to judge.

Edit:
If you want to drill down even further, it results in:

grafik
grafik1890×1082 176 KB

Edit 2:
Nice command line options. Even offers statistics. Should have checked before.
The entire glory:

tklipper@bigtreetech-cb1 ~/klipper $ ruff check --select ALL --statistics
warning: `one-blank-line-before-class` (D203) and `no-blank-line-before-class` (D211) are incompatible. Ignoring `one-blank-line-before-class`.
warning: `multi-line-summary-first-line` (D212) and `multi-line-summary-second-line` (D213) are incompatible. Ignoring `multi-line-summary-second-line`.
5139    Q000            [*] bad-quotes-inline-string
4254    ANN001          [ ] missing-type-function-argument
2348    ANN201          [ ] missing-return-type-undocumented-public-function
1851    D102            [ ] undocumented-public-method
 803    UP031           [*] printf-string-formatting
 767    ANN202          [ ] missing-return-type-private-function
 433    D103            [ ] undocumented-public-function
 407    D101            [ ] undocumented-public-class
 397    ANN204          [ ] missing-return-type-special-method
 376    D107            [ ] undocumented-public-init
 263    COM812          [*] missing-trailing-comma
 254    PLR2004         [ ] magic-value-comparison
 247    ARG002          [ ] unused-method-argument
 247    N802            [ ] invalid-function-name
 242    TRY003          [ ] raise-vanilla-args
 240    EM101           [*] raw-string-in-exception
 188    D100            [ ] undocumented-public-module
 187    N806            [ ] non-lowercase-variable-in-function
 155    I001            [*] unsorted-imports
 153    UP032           [*] f-string
 126    N815            [ ] mixed-case-variable-in-class-scope
 124    FBT002          [ ] boolean-default-value-positional-argument
 115    FBT003          [ ] boolean-positional-value-in-call
 104    E401            [*] multiple-imports-on-one-line
  89    D212            [*] multi-line-summary-first-line
  82    G002            [ ] logging-percent-format
  74    PLW0603         [ ] global-statement
  69    SLF001          [ ] private-member-access
  66    B904            [ ] raise-without-from-inside-except
  64    PTH123          [ ] builtin-open
  63    E722            [ ] bare-except
  59    N801            [ ] invalid-class-name
  56    C901            [ ] complex-structure
  56    F841            [*] unused-variable
  53    T201            [*] print
  53    D205            [ ] blank-line-after-summary
  51    ERA001          [*] commented-out-code
  47    PTH118          [ ] os-path-join
  47    F401            [ ] unused-import
  44    PLW2901         [ ] redefined-loop-name
  43    B007            [ ] unused-loop-control-variable
  42    PLR0912         [ ] too-many-branches
  40    UP024           [*] os-error-alias
  37    SIM115          [ ] open-file-with-context-handler
  34    D401            [ ] non-imperative-mood
  30    D200            [*] fits-on-one-line
  29    E741            [ ] ambiguous-variable-name
  29    RUF005          [*] collection-literal-concatenation
  26    RET505          [ ] superfluous-else-return
  26    PTH120          [ ] os-path-dirname
  24    SIM105          [ ] suppressible-exception
  24    PLR0913         [ ] too-many-arguments
  23    PLR0915         [ ] too-many-statements
  22    INP001          [ ] implicit-namespace-package
  22    RET503          [*] implicit-return
  21    RET504          [*] unnecessary-assign
  21    ARG001          [ ] unused-function-argument
  21    PTH110          [ ] os-path-exists
  20    SIM102          [ ] collapsible-if
  20    SIM208          [*] double-negation
  19    ICN001          [ ] unconventional-import-alias
  19    D400            [ ] ends-in-period
  19    D415            [ ] ends-in-punctuation
  18    TD002           [ ] missing-todo-author
  18    TD003           [ ] missing-todo-link
  18    PD901           [ ] pandas-df-variable-name
  18    UP015           [*] redundant-open-modes
  17    ANN002          [ ] missing-type-args
  17    ISC002          [ ] multi-line-implicit-string-concatenation
  17    RUF012          [ ] mutable-class-default
  16    ANN003          [ ] missing-type-kwargs
  16    BLE001          [ ] blind-except
  15    SIM108          [ ] if-else-block-instead-of-if-exp
  15    E703            [*] useless-semicolon
  15    UP008           [*] super-call-with-parameters
  14    S603            [ ] subprocess-without-shell-equals-true
  14    COM819          [*] prohibited-trailing-comma
  13    B006            [*] mutable-argument-default
  13    TRY002          [ ] raise-vanilla-class
  12    ARG005          [ ] unused-lambda-argument
  11    FIX003          [ ] line-contains-xxx
  11    TD001           [ ] invalid-todo-tag
  11    TD004           [ ] missing-todo-colon
  11    N818            [ ] error-suffix-on-exception-name
  11    E721            [ ] type-comparison
  11    D105            [ ] undocumented-magic-method
  11    UP034           [*] extraneous-parentheses
  10    EM103           [*] dot-format-in-exception
  10    PIE810          [*] multiple-starts-ends-with
   9    S108            [ ] hardcoded-temp-file
   9    S110            [ ] try-except-pass
   9    ISC003          [ ] explicit-string-concatenation
   9    PERF203         [ ] try-except-in-loop
   9    E402            [ ] module-import-not-at-top-of-file
   9    UP004           [*] useless-object-inheritance
   8    N803            [ ] invalid-argument-name
   8    PERF401         [ ] manual-list-comprehension
   8    E731            [*] lambda-assignment
   8    F821            [ ] undefined-name
   7    A001            [ ] builtin-variable-shadowing
   7    FIX002          [ ] line-contains-todo
   7    G004            [ ] logging-f-string
   7    PTH111          [ ] os-path-expanduser
   7    E701            [ ] multiple-statements-on-one-line-colon
   7    UP007           [*] non-pep604-annotation
   6    S324            [ ] hashlib-insecure-hash-function
   6    A002            [ ] builtin-argument-shadowing
   6    G003            [ ] logging-string-concat
   6    SIM223          [*] expr-and-false
   6    PTH100          [ ] os-path-abspath
   6    PTH108          [ ] os-unlink
   6    E501            [ ] line-too-long
   6    E711            [*] none-comparison
   6    D204            [*] one-blank-line-after-class
   6    D300            [*] triple-single-quotes
   6    PLR5501         [*] collapsible-else-if
   6    UP027           [*] unpacked-list-comprehension
   6    TRY300          [ ] try-consider-else
   6    TRY400          [*] error-instead-of-exception
   5    DTZ004          [ ] call-datetime-utcfromtimestamp
   5    EXE001          [ ] shebang-not-executable
   5    PYI024          [ ] collections-named-tuple
   5    Q002            [*] bad-quotes-docstring
   5    RET506          [ ] superfluous-else-raise
   5    SIM118          [*] in-dict-keys
   5    TD005           [ ] missing-todo-description
   5    PTH119          [ ] os-path-basename
   5    PLR0911         [ ] too-many-return-statements
   4    S605            [ ] start-process-with-a-shell
   4    C419            [*] unnecessary-comprehension-in-call
   4    PIE808          [*] unnecessary-range-start
   4    RET502          [*] implicit-return-value
   4    SIM113          [ ] enumerate-for-loop
   4    PTH202          [ ] os-path-getsize
   4    PD011           [ ] pandas-use-of-dot-values
   4    D104            [ ] undocumented-public-package
   4    UP006           [*] non-pep585-annotation
   3    ANN206          [ ] missing-return-type-class-method
   3    C413            [*] unnecessary-call-around-sorted
   3    C416            [*] unnecessary-comprehension
   3    SIM210          [*] if-expr-with-true-false
   3    SIM300          [*] yoda-conditions
   3    TID252          [*] relative-imports
   3    PTH113          [ ] os-path-isfile
   3    D106            [ ] undocumented-public-nested-class
   3    PLR1704         [ ] redefined-argument-from-local
   3    PLW0602         [ ] global-variable-not-assigned
   3    UP009           [*] utf8-encoding-declaration
   3    UP010           [*] unnecessary-future-import
   3    TRY301          [ ] raise-within-try
   2    EM102           [*] f-string-in-exception
   2    FIX004          [ ] line-contains-hack
   2    SIM103          [*] needless-bool
   2    SIM910          [*] dict-get-with-none-default
   2    PTH104          [ ] os-rename
   2    PTH122          [ ] os-path-splitext
   2    PTH204          [ ] os-path-getmtime
   2    PTH207          [ ] glob
   2    W605            [*] invalid-escape-sequence
   2    D402            [ ] no-signature
   2    D407            [*] dashed-underline-after-section
   2    D413            [*] blank-line-after-last-section
   2    PLE1307         [ ] bad-string-format-type
   2    PLR1714         [*] repeated-equality-comparison
   2    PLR1722         [*] sys-exit-alias
   2    UP003           [*] type-of-primitive
   2    RUF015          [*] unnecessary-iterable-allocation-for-first-element
   1                    [ ] syntax-error
   1    ASYNC230        [ ] blocking-open-call-in-async-function
   1    S101            [ ] assert
   1    S307            [ ] suspicious-eval-usage
   1    S602            [ ] subprocess-popen-with-shell-equals-true
   1    S701            [ ] jinja2-autoescape-false
   1    B018            [ ] useless-expression
   1    C408            [*] unnecessary-collection-call
   1    RSE102          [*] unnecessary-paren-on-raise-exception
   1    RET507          [ ] superfluous-else-continue
   1    SIM101          [*] duplicate-isinstance-call
   1    SIM112          [ ] uncapitalized-environment-variables
   1    SIM114          [*] if-with-same-arms
   1    SIM117          [ ] multiple-with-statements
   1    TD006           [*] invalid-todo-capitalization
   1    PTH101          [ ] os-chmod
   1    PTH102          [ ] os-mkdir
   1    PTH103          [ ] os-makedirs
   1    PTH107          [ ] os-remove
   1    PTH109          [ ] os-getcwd
   1    PTH114          [ ] os-path-islink
   1    PTH116          [ ] os-stat
   1    N999            [ ] invalid-module-name
   1    PERF102         [*] incorrect-dict-iterator
   1    PERF402         [ ] manual-list-copy
   1    E713            [*] not-in-test
   1    D405            [*] capitalize-section-name
   1    D416            [*] section-name-ends-in-colon
   1    F402            [ ] import-shadowed-by-loop-var
   1    F811            [ ] redefined-while-unused
   1    PLR0402         [*] manual-from-import
   1    PLR1711         [*] useless-return
   1    UP020           [*] open-alias
   1    FURB136         [*] if-expr-min-max
   1    RUF006          [ ] asyncio-dangling-task
   1    TRY201          [*] verbose-raise
   1    TRY401          [ ] verbose-log-message
tklipper@bigtreetech-cb1 ~/klipper $
2 Likes
6

I’m aware that not all “errors” (and especially not all “errors” reported by --select ALL) are useful.

I fully agree that things like “multiple imports on one line” can be safely ignored and added to the config file’s ignore list.

My intention was to start this process without using --select ALL specifically to make the introduction easier. And I’m not proposing the sudden introduction of ruff as a blocking step in CI.

Dropping a “ruff.toml” in the project’s root will have no effect on those that don’t explicitly run it or have it in their editor configuration.

7

That being said, should the discussion about the specific error codes to fix/ignore be hosted here or on the pull request ?

8

But back to the basics, this, as a whole, is a python 3 project, right? Is it a specific version of python 3? It should be included in the config as well.

9

The shared spreadsheet and the graph above have just been run without any command line options.
I just played around with it out of pure interest.

As far as my understanding goes, Klipper currently is still maintaining Python 2 backwards compatibility, as many installations and even the default installation procedure in the documentation is still based on Python 2.

But for a final call and for the rest of your questions, @koconnor is the relevant authority to answer them.