[RFC] scripts: ci: check_compliance: Add python lint/format check

[pdgendt]

Introduction

As the codebase and contributions keep growing, so does all the python tooling.
This PR is intended to make python scripts more uniform and safe.

Proposed change

Add ruff as a linter and formatter for python files, and verify (new) files in CI.

Detailed RFC

Some hightlights:

• ruff is a drop-in replacement for many popular python tools, and it is super fast.
• The formatter output is PEP8 compliant and produces nearly identical output to black
• Can fix issues for you
• ...

Current enabled linter rules:

• flake8-bugbear
• pycodestyle
• pyflakes
• isort (black profile)
• flake8-simplify
• pyupgrade

Many more can be added/enabled but this is a solid baseline.

Concerns

Existing files do not comply, so they are explicitly excluded. Ideally these are gradually converted to comply.

$ ruff check
Found 3708 errors.

$ ruff format --check
379 files would be reformatted, 34 files already formatted

Formatting is obviously opinionated, but I do think for python we should follow the popular kids, which are black and ruff.

[nordicjm]

Why add exclusions? If someone adds a new file e.g. a runner from an external source, they would have to fix the file, but if an existing runner which does not comply is excluded, that can be updated forever going forwards without having to comply? Or is the idea that new exclusions would be added, in which case what would be the point of having the check if everything is excluded?

[pdgendt]

Why add exclusions? If someone adds a new file e.g. a runner from an external source, they would have to fix the file, but if an existing runner which does not comply is excluded, that can be updated forever going forwards without having to comply? Or is the idea that new exclusions would be added, in which case what would be the point of having the check if everything is excluded?

This is up for debate of course, but the idea is to force new files to comply, while we gradually convert the existing ones. It could be brought up during a review to update a file you are touching to comply.

[nordicjm]

Why add exclusions? If someone adds a new file e.g. a runner from an external source, they would have to fix the file, but if an existing runner which does not comply is excluded, that can be updated forever going forwards without having to comply? Or is the idea that new exclusions would be added, in which case what would be the point of having the check if everything is excluded?

This is up for debate of course, but the idea is to force new files to comply, while we gradually convert the existing ones. It could be brought up during a review to update a file you are touching to comply.

This is something I asked about before because one of the files added from an external source, a uf2 one if I remember right, had some oddities about it, and someone said the file shouldn't be fixed because whilst it was being added to zephyr, it was not a zephyr file, and you then lose the ability to diff etc.

[pdgendt]

This is something I asked about before because one of the files added from an external source, a uf2 one if I remember right, had some oddities about it, and someone said the file shouldn't be fixed because whilst it was being added to zephyr, it was not a zephyr file, and you then lose the ability to diff etc.

I think this can be an exception, but that's what the exclude lists are for.

[hakehuang]

• Can fix issues for you

can we use ruff to fix the issues in this PR?

[pdgendt]

Rebased on main

[pdgendt]

Update:

• Ignore SIM108 to allow if-else blocks instead of forcing ternary operator

[fabiobaltieri]

@pdgendt where do you want to go from here? It's titled and tagged as RFC but arguably good to go. Maybe arch wg? (though it's not really an architectural change but seems like any change with wide impact goes there these days)

[pdgendt]

I was thinking the same thing, wanted to bring this up during some workgroup meeting. Could be for arch or process? Maybe we need a tooling working group?

CC @carlescufi @nashif any suggestions?

[fabiobaltieri]

I think process would be the "correct-er" one but if you want visibility arch is the one to go, I suggest bringing it up there.

[nashif]

are we going to keep pylint?

[pdgendt]

are we going to keep pylint?

There's no conflict between these two, there is some overlap but I haven't added any of the PL rules in this PR.

We might favor enabling the PL rules from ruff and replace pylint with an actual type checker. But that's outside of the scope of this PR.

[nashif]

There's no conflict between these two, there is some overlap but I haven't added any of the PL rules in this PR.

We might favor enabling the PL rules from ruff and replace pylint with an actual type checker. But that's outside of the scope of this PR.

but it is suboptimal if we have two linters doing python, at leasr from a developer experience. For example, if there is a violation that is detected by both, will we get two warnings?

[pdgendt]

but it is suboptimal if we have two linters doing python, at leasr from a developer experience. For example, if there is a violation that is detected by both, will we get two warnings?

Presumably, yes. But I haven't looked into the overlaps of the current configurations, this can be optimized on the go.

As you can see from the excludes file, there are a lot of linter issues that pylint hasn't reported on.

[fabiobaltieri]

I think that before doing this (or at least as part of this) we should at least batch fix the files that are obviously not imported and meant to track an external project, I guess anything under arch, doc, samples, script, tests...

[pdgendt]

I think that before doing this (or at least as part of this) we should at least batch fix the files that are obviously not imported and meant to track an external project, I guess anything under arch, doc, samples, script, tests...

I was going to do it the other way around, this PR is to prevent any new issues from this point forward.

[fabiobaltieri]

I was going to do it the other way around, this PR is to prevent any new issues from this point forward.

Yup that makes sense.

[fabiobaltieri]

• summarize the issue: linter+formatter check for any new python file
• @nashif: asks how this behaves with the current files (out of compliance), current files/exclusions woul
  d not trigger
• @pdgendt: most of these can be fixed esily, will do some folowup to look into this, the tool has an option
  to change the file inline and also do only "safe" changes
• no conerns on trying this for new files