feature(pre-commit): replace pylint with ruff

[fruch]

Ruff is rust base linter for python that work incredibly fast since pylint speed is causing use issue on SCT CI we should start using ruff, for now it uses the pylint set of rules it has implemeted and we can slow extand it's configuration to use more of the available rules set it has

Ref: https://github.com/charliermarsh/ruff Ref: https://www.youtube.com/watch?v=jeoL4qsSLbE

PR pre-checks (self review)

• [ ] I followed KISS principle and best practices
• [ ] I didn't leave commented-out/debugging code
• [ ] I added the relevant backport labels
• [ ] New configuration option are added and documented (in sdcm/sct_config.py)
• [ ] I have added tests to cover my changes (Infrastructure only - under unit-test/ folder)
• [ ] All new and existing unit tests passed (CI)
• [ ] I have updated the Readme/doc folder accordingly (if needed)

[fruch]

@roydahan @soyacz @vponomaryov @juliayakovlev @k0machi

since this gonna affect the daily work of all of us, it would help if you can take it for a walk, and see how it work for you.

[vponomaryov]

@fruch I think we should skip rules making us replace single-quote with double quote symbols and adding trailing comma. Why? Because it is all over our codebase.

Then, it would be much easier to try it out.

[fruch]

@fruch I think we should skip rules making us replace single-quote with double quote symbols and adding trailing comma. Why? Because it is all over our codebase.

Then, it would be much easier to try it out.

You suggesting it for a way to try it out only ? to see how fast it is ?

Or skip it since it's making too many changes to the code ?

[vponomaryov]

@fruch I think we should skip rules making us replace single-quote with double quote symbols and adding trailing comma. Why? Because it is all over our codebase. Then, it would be much easier to try it out.

You suggesting it for a way to try it out only ? to see how fast it is ?

Or skip it since it's making too many changes to the code ?

I have no doubts that it is fast. It is about second assumption, but not completely it. WIth much fewer changes it will be possible to

• See whole set of changes we need to do without those 2 already mentioned
• Apply it for trying out anytime on almost any master branch state. Now it is already out of compatibility.

[soyacz]

@fruch I think we should skip rules making us replace single-quote with double quote symbols and adding trailing comma.

I'm opting also to skip this - we won't get too much from this change but it will cause lot's of troubles when backporting or when checking 'git blame'

[fruch]

@fruch I think we should skip rules making us replace single-quote with double quote symbols and adding trailing comma.

I'm opting also to skip this - we won't get too much from this change but it will cause lot's of troubles when backporting or when checking 'git blame'

we have lot of other changes that would affect git blame, cause of formatting has change, those would be a drop in the sea.

and changing the tool/formatter does have it's price, backporting would be hard, regregless of we remove one or two rules out.

[fruch]

@soyacz @vponomaryov

I've remove the double quoteand trailing comma, but still the list of change is quite huge. I think it's acceptable, and a one off that would happen on any major change of formatter/linter.

[vponomaryov]

The precommit fails:

17:28:11  docker/alternator-dns/dns_server.py:1:1: EXE001 Shebang is present but file is not executable
17:28:11  Found 1 error.

[vponomaryov]

@fruch Added more comments after looking at several python files. There are a lot of cases where transformation is ugly because the structure doesn't get changed.

So, multiplying this observation onto the 400+ files , which is insane to review as part of a single PR, I think we should not merge such a big change.

Proposal: I think, better would be to add ruff as second linter where almost all python files are exlcuded except bunch of fixed ones. And then we can extend the coverage fixing files in bunches. Something like 40 commit with 10 python files...

@soyacz WDYT?

[fruch]

@fruch Added more comments after looking at several python files. There are a lot of cases where transformation is ugly because the structure doesn't get changed.

So, multiplying this observation onto the 400+ files , which is insane to review as part of a single PR, I think we should not merge such a big change.

Proposal: I think, better would be to add ruff as second linter where almost all python files are exlcuded except bunch of fixed ones. And then we can extend the coverage fixing files in bunches. Something like 40 commit with 10 python files...

@soyacz WDYT?

We can try black, but it's gonna be a similar experience, it's a very opinionated formatter, and people might not like the choices it would do.

But that's the deal with those formatters, you trust them to eliminate discussions about format.

As for splitting, it might be doable, but gonna be a bit tedious, like we did in dtest when introduced pylint

[soyacz]

@fruch Added more comments after looking at several python files. There are a lot of cases where transformation is ugly because the structure doesn't get changed.

So, multiplying this observation onto the 400+ files , which is insane to review as part of a single PR, I think we should not merge such a big change.

Proposal: I think, better would be to add ruff as second linter where almost all python files are exlcuded except bunch of fixed ones. And then we can extend the coverage fixing files in bunches. Something like 40 commit with 10 python files...

@soyacz WDYT?

I think we should not format with ruff for now. My proposal would to leave autopep6 and replace pylint only. And put in pyproject.toml:

[tool.ruff]
lint.select = ["PL", "YTT", "BLE", "I", "UP", "F401", "F402", "F821", "T10", "EXE", "TID"]

lint.ignore = ["E501", "PLR2004", "COM812", "BLE001", "UP031"]

target-version = "py310"

respect-gitignore = true
force-exclude = true
line-length = 112

[tool.ruff.lint.pylint]
max-args = 20
max-statements = 180
max-branches = 81

and fix manually errors (only around 200 and some are valid ones)

[fruch]

@fruch Added more comments after looking at several python files. There are a lot of cases where transformation is ugly because the structure doesn't get changed. So, multiplying this observation onto the 400+ files , which is insane to review as part of a single PR, I think we should not merge such a big change. Proposal: I think, better would be to add ruff as second linter where almost all python files are exlcuded except bunch of fixed ones. And then we can extend the coverage fixing files in bunches. Something like 40 commit with 10 python files... @soyacz WDYT?

I think we should not format with ruff for now. My proposal would to leave autopep6 and replace pylint only. And put in pyproject.toml:

[tool.ruff]
lint.select = ["PL", "YTT", "BLE", "I", "UP", "F401", "F402", "F821", "T10", "EXE", "TID"]

lint.ignore = ["E501", "PLR2004", "COM812", "BLE001", "UP031"]

target-version = "py310"

respect-gitignore = true
force-exclude = true
line-length = 112

[tool.ruff.lint.pylint]
max-args = 20
max-statements = 180
max-branches = 81

and fix manually errors (only around 200 and some are valid ones)

I've remove the formatter change, and I'll see how we introduce it in smaller batches

I'm not extending the number of pylint, just to minimize the change, we it the same figured as we had before., also BLE001 (it's the only one in BLE)

[soyacz]

I'd leave pylint comments to minimize the change. Removing them is similar to reformat which we wanted to avoid.

[vponomaryov]

I'd leave pylint comments to minimize the change. Removing them is similar to reformat which we wanted to avoid.

I proposed to leave only first commit in this PR. Then, second PR with pylint comments removal. And then all other, separately.

Doing so, we could merge the first commit today. Also, doing so we can improve places where the linter does strange things such as 2 string objects instead of one on a single line...

[fruch]

I'd leave pylint comments to minimize the change. Removing them is similar to reformat which we wanted to avoid.

I proposed to leave only first commit in this PR. Then, second PR with pylint comments removal. And then all other, separately.

Doing so, we could merge the first commit today. Also, doing so we can improve places where the linter does strange things such as 2 string objects instead of one on a single line...

I fail to see how splitting it to multiple PR would help, just for reviewing it easier ?

[vponomaryov]

I'd leave pylint comments to minimize the change. Removing them is similar to reformat which we wanted to avoid.

I proposed to leave only first commit in this PR. Then, second PR with pylint comments removal. And then all other, separately. Doing so, we could merge the first commit today. Also, doing so we can improve places where the linter does strange things such as 2 string objects instead of one on a single line...

I fail to see how splitting it to multiple PR would help, just for reviewing it easier ?

Just? Possibility to have full review will allow to catch lots of undesired changes where it is better to update some places manually.

[fruch]

I'd leave pylint comments to minimize the change. Removing them is similar to reformat which we wanted to avoid.

I proposed to leave only first commit in this PR. Then, second PR with pylint comments removal. And then all other, separately. Doing so, we could merge the first commit today. Also, doing so we can improve places where the linter does strange things such as 2 string objects instead of one on a single line...

I fail to see how splitting it to multiple PR would help, just for reviewing it easier ?

Just? Possibility to have full review will allow to catch lots of undesired changes where it is better to update some places manually.

If you are gonna read it all line by line, every signal change here. I'll just merge it now and that's it.

that's not the point of such a change, it's not to review the changes, themselves but the tool / rules we are using.

[vponomaryov]

If you are gonna read it all line by line, every signal change here. I'll just merge it now and that's it.

that's not the point of such a change, it's not to review the changes, themselves but the tool / rules we are using.

I don't see arguments against creating first PR with first commit and further keeping adequate number of changes per PR. I see only threat to merge if I review code in the place where it is the main goal - to review code.

We already could merge first one or two commits with full confidence in the made changes.

[fruch]

If you are gonna read it all line by line, every signal change here. I'll just merge it now and that's it. that's not the point of such a change, it's not to review the changes, themselves but the tool / rules we are using.

I don't see arguments against creating first PR with first commit and further keeping adequate number of changes per PR. I see only threat to merge if I review code in the place where it is the main goal - to review code.

We already could merge first one or two commits with full confidence in the made changes.

Separating those make no sense, and would me that we'll have almost no lint on master until the rest of those would be introduced.

or we have confidence in the tool we are introducing, or we don't, I don't see a point review every single automated change that the tool was doing (which 99% of those change are)

[vponomaryov]

@fruch need to rebase it.