GitHub Action to lint Python code with ruff #1771

cclauss · 2023-03-09T22:55:49Z

Ruff supports over 500 lint rules and can be used to replace Flake8 (plus dozens of plugins), isort, pydocstyle, yesqa, eradicate, pyupgrade, and autoflake, all while executing (in Rust) tens or hundreds of times faster than any individual tool.

The ruff Action uses minimal steps to run in ~10 seconds, rapidly providing intuitive GitHub Annotations to contributors.

Describe the changes

The related issue or a description of the bug or feature that this PR addresses.

Tasks

Unit tests added that reproduce the issue or prove feature is working
Fix or feature added
Code typed and mypy shows no errors
Documentation updated (if relevant)
- No warnings during build
- Preview link (CircleCI -> Artifacts -> doc/_build/html/index.html)
Unit tests passing and overall coverage the same or better

codecov · 2023-03-10T06:43:45Z

Codecov Report

Merging #1771 (8a4c4d1) into master (2709ff6) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1771      +/-   ##
==========================================
+ Coverage   97.41%   97.43%   +0.01%     
==========================================
  Files          66       66              
  Lines       10839    10839              
==========================================
+ Hits        10559    10561       +2     
+ Misses        280      278       -2

Impacted Files	Coverage Δ
pydicom/sr/_snomed_dict.py	`100.00% <ø> (ø)`
pydicom/util/leanread.py	`88.63% <100.00%> (ø)`

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

darcymason · 2023-03-10T16:02:12Z

I want be able to take a good look at this for a couple of days, probably. But a quick question - can you elaborate a bit on what this does better or differently than what we have currently? You mentioned speed and the intuitive annotations, but does this report on new/different categories of linting problems than our existing setup?

cclauss · 2023-03-10T16:07:09Z

I believe that the front page of https://beta.ruff.rs explains the advantages quite succinctly. If we did ruff --select=ALL . then yes, it would report on many new/different categories of lining problems than your existing setup.

darcymason · 2023-03-10T16:27:36Z

I believe that the front page of https://beta.ruff.rs explains the advantages quite succinctly. If we did ruff --select=ALL . then yes, it would report on many new/different categories of lining problems than your existing setup.

Okay, thanks for those links. I'll review them when I get a chance.

mrbean-bremen · 2023-03-10T19:18:55Z

.github/workflows/ruff.yml

+    - uses: actions/checkout@v3
+    - run: pip install --user ruff
+    - run: ruff --ignore=E402,F401,F403,F405,F541,F601,F811,F841
+                --format=github --line-length=214 --target-version=py37 .


As far as I understand, this replaces flake8, so it probably makes sense to remove the flake8 check if this will be used.

I wonder about that line length of 214 requirement. We also use black, which uses 88 as the maximum line length, which ruff also seems to use, so why this setting? In case it doesn't understand the noqa comment, it would be better to switch off the check for line length, as it is already done by black anyway.

Also, why all the exclusions? Apart from E402 (module-import-not-at-top-of-file) I see no reason for the rest.

https://github.com/pydicom/pydicom/blob/master/Makefile#L32-L34

Sorry, I don't understand what you mean.
I can see that this makefile is quite outdated and should probably be adapted or removed, though :)

Let's leverage --format=github to show where the errors are.

Please review the first ten errors in the code listing... https://github.com/pydicom/pydicom/pull/1771/files

Let's agree that line-length is currently different under psf/black (best effort formatter) and ruff (rules-based linter) but our goal is to make them the same.

The difference because the former is guidance for a tool that formats code but not comments, docstrings, etc., and is deliberately timid about reformatting strings while the latter can be used as a trigger for one or more good-first-issues for interested contributors to shorten long lines that psf/black does not autofix.

About the first ten errors: I think these are all things that can be either easily fixed, or these lines should get an exclusion comment. I just think it makes more sense to fix them instead of disabling the warnings.
As for line-length: as far as I could see, these are from generated code. It would make sense just to exclude these files from the check by a file-wide exclusion comment, if that is possible. The same is true for unused imports in __init__.py.

Anyway, I'm not saying that you have to do all of it now - that is something that can also be handled later by somebody else. In this case I would like to at least remove the redundant flake8 check (it is redundant, right?), and write a new issue for enabling and fixing these warnings.

pyproject.toml added with appropriate settings and #1776 add to cover follow-on tasks.

[Ruff](https://beta.ruff.rs/) supports [over 500 lint rules](https://beta.ruff.rs/docs/rules) including bandit, isort, pylint, pyupgrade, and flake8 plus its plugins and is written in Rust for speed. The `ruff` Action uses minimal steps to run in ~10 seconds, rapidly providing intuitive GitHub Annotations to contributors. ![image](https://user-images.githubusercontent.com/3709715/223758136-afc386d2-70aa-4eff-953a-2c2d82ceea23.png)

mrbean-bremen · 2023-03-24T18:36:07Z

.github/workflows/ruff.yml

+    steps:
+    - uses: actions/checkout@v3
+    - run: pip install --user ruff
+    - run: ruff --format=github .


You left in the flake8 workflow - do you think it is still needed?

It is wired into https://github.com/reviewdog/action-flake8 and I cannot tell if you need reviewdog.

Well, reviewdoc does the same as ruff, e.g. create annotation in the code, so if ruff is better suited, we don't need it anymore. I see that you have removed it already.
I may have a look at the differences between the two later, but your changes look good to me so far.
Will wait for @darcymason for his opinion on the change.

Hi all, I have limited time right now, but I'll try to take a better look later today.

@mrbean-bremen, I wasn't sure if there was anything else you wanted to review, but I'll merge in and we can adapt in other PR(s) as needed...

That's ok, I'm happy with that. There is still #1776 for improvements.

darcymason

Okay, had a pretty good look through. Thanks for this - it does make all this much leaner and looks easier to fine-tune as well.

cclauss marked this pull request as draft March 9, 2023 23:01

cclauss marked this pull request as ready for review March 9, 2023 23:14

mrbean-bremen reviewed Mar 10, 2023

View reviewed changes

cclauss force-pushed the patch-1 branch 2 times, most recently from b1e8dbf to 4912ef5 Compare March 24, 2023 13:23

cclauss mentioned this pull request Mar 24, 2023

Improve Python linting with ruff #1776

Open

8 tasks

cclauss force-pushed the patch-1 branch from 4912ef5 to 5dacc5f Compare March 24, 2023 14:10

mrbean-bremen reviewed Mar 24, 2023

View reviewed changes

cclauss added 3 commits March 24, 2023 21:40

Update pr-run-linters.yml

670a96f

Delete ruff.yml

569f678

Update pr-run-linters.yml

8a4c4d1

darcymason approved these changes Mar 26, 2023

View reviewed changes

darcymason merged commit f7fdf0f into pydicom:master Mar 26, 2023
25 checks passed

cclauss deleted the patch-1 branch March 26, 2023 13:13

This was referenced Dec 26, 2023

Change Python formatting: black → ruff #1981

Open

[MRG] Apply Repo-Review suggestions #1982

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GitHub Action to lint Python code with ruff #1771

GitHub Action to lint Python code with ruff #1771

cclauss commented Mar 9, 2023 •

edited

codecov bot commented Mar 10, 2023 •

edited

darcymason commented Mar 10, 2023

cclauss commented Mar 10, 2023 •

edited

darcymason commented Mar 10, 2023

mrbean-bremen Mar 10, 2023

cclauss Mar 10, 2023

mrbean-bremen Mar 10, 2023

cclauss Mar 12, 2023

cclauss Mar 12, 2023 •

edited

mrbean-bremen Mar 12, 2023

cclauss Mar 24, 2023

mrbean-bremen Mar 24, 2023

cclauss Mar 24, 2023

mrbean-bremen Mar 24, 2023

darcymason Mar 25, 2023

darcymason Mar 26, 2023

mrbean-bremen Mar 26, 2023

darcymason left a comment

GitHub Action to lint Python code with ruff #1771

GitHub Action to lint Python code with ruff #1771

Conversation

cclauss commented Mar 9, 2023 • edited

Describe the changes

Tasks

codecov bot commented Mar 10, 2023 • edited

Codecov Report

darcymason commented Mar 10, 2023

cclauss commented Mar 10, 2023 • edited

darcymason commented Mar 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cclauss Mar 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darcymason left a comment

Choose a reason for hiding this comment

cclauss commented Mar 9, 2023 •

edited

codecov bot commented Mar 10, 2023 •

edited

cclauss commented Mar 10, 2023 •

edited

cclauss Mar 12, 2023 •

edited