Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

build: replace Python linter flake8 with ruff #47519

Merged
merged 3 commits into from
Apr 14, 2023
Merged

build: replace Python linter flake8 with ruff #47519

merged 3 commits into from
Apr 14, 2023

Conversation

cclauss
Copy link
Contributor

@cclauss cclauss commented Apr 12, 2023 •
edited
Loading

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 ~5 seconds, rapidly providing intuitive GitHub Annotations to contributors.

image

@charliermarsh

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 12, 2023 •
edited by cclauss
Loading

Review requested:

  • @nodejs/gyp
  • @nodejs/python

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. dont-land-on-v14.x gyp Issues and PRs related to the GYP tool and .gyp build files i18n-api Issues and PRs related to the i18n implementation. icu Issues and PRs related to the ICU dependency. meta Issues and PRs related to the general management of the project. needs-ci PRs that need a full CI run. openssl Issues and PRs related to the OpenSSL dependency. tls Issues and PRs related to the tls subsystem. tools Issues and PRs related to the tools directory. labels Apr 12, 2023
@cclauss cclauss force-pushed the ruff branch 2 times, most recently from 6bba85d to 9da5ea6 Compare April 12, 2023 01:25
@cclauss cclauss changed the title ci: replace Python linter flake8 with ruff build: replace Python linter flake8 with ruff Apr 12, 2023
@cclauss cclauss requested review from gengjiawen and ryzokuken April 12, 2023 01:55
anonrig
anonrig approved these changes Apr 12, 2023
View reviewed changes
Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a fan of Ruff. I was planning on doing this change. Happy to see this. Can you share the execution time difference with this change?

@Trott Trott added the python PRs and issues that require attention from people who are familiar with Python. label Apr 12, 2023
@gengjiawen gengjiawen added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 12, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 12, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/51148/

@cclauss
Copy link
Contributor Author

cclauss commented Apr 12, 2023

@anonrig Execution time is less than a 1/10th of a second.

node git:(ruff) % time ruff . 

ruff .  0.08s user 0.09s system 85% cpu 0.197 total

@gengjiawen
Copy link
Member

gengjiawen commented Apr 12, 2023 •
edited by cclauss
Loading

node-test-linter failed, not sure extra build steps needed @nodejs/build-infra

@cclauss
Copy link
Contributor Author

cclauss commented Apr 12, 2023

These errors are caused by the mods that are made in the Makefile. Ruff is a Rust exe while flake8 was a Python command line tool. I will convert this PR to DRAFT while I work to fix...

@cclauss cclauss marked this pull request as draft April 12, 2023 09:28
@cclauss cclauss marked this pull request as ready for review April 12, 2023 11:14
richardlau
richardlau reviewed Apr 12, 2023
View reviewed changes
Makefile Outdated Show resolved Hide resolved
@nodejs nodejs deleted a comment from tungmeoo Apr 12, 2023
@github-actions github-actions bot mentioned this pull request Apr 13, 2023
Open
36 tasks
@richardlau richardlau added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. request-ci Add this label to start a Jenkins CI on a PR. labels Apr 14, 2023
@richardlau
Copy link
Member

The five failures look unrelated but perhaps I am missing something.

We have a number of flaky tests (more since the recent V8 update).

@MoLow MoLow removed tls Issues and PRs related to the tls subsystem. openssl Issues and PRs related to the OpenSSL dependency. i18n-api Issues and PRs related to the i18n implementation. gyp Issues and PRs related to the GYP tool and .gyp build files icu Issues and PRs related to the ICU dependency. labels Apr 14, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 14, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/51254/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/51258/

gengjiawen
gengjiawen approved these changes Apr 14, 2023
View reviewed changes
Copy link
Member

@gengjiawen gengjiawen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent job

@richardlau richardlau added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 14, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 14, 2023
@nodejs-github-bot nodejs-github-bot merged commit 4c22414 into nodejs:main Apr 14, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 4c22414

@cclauss cclauss deleted the ruff branch April 14, 2023 16:52
@cclauss
Copy link
Contributor Author

cclauss commented Apr 14, 2023 •
edited
Loading

Thanks all! -- Its a team sport ;-) If you are involved nodejs/build, please give this a review so I can complete the tour...

This was referenced Apr 15, 2023
Open
Open
targos pushed a commit that referenced this pull request May 2, 2023
@cclauss @targos
build: replace Python linter flake8 with ruff
78c0824 
PR-URL: #47519
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
@targos targos mentioned this pull request May 2, 2023
Merged
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
@cclauss @danielleadams
build: replace Python linter flake8 with ruff
9845e65 
PR-URL: #47519
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023
@cclauss @MoLow
build: replace Python linter flake8 with ruff
48468c4 
PR-URL: nodejs#47519
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
@danielleadams danielleadams mentioned this pull request Jul 10, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MoLow MoLow MoLow approved these changes

@richardlau richardlau richardlau approved these changes

@gengjiawen gengjiawen gengjiawen approved these changes

@anonrig anonrig anonrig approved these changes

@ryzokuken ryzokuken Awaiting requested review from ryzokuken

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. meta Issues and PRs related to the general management of the project. needs-ci PRs that need a full CI run. python PRs and issues that require attention from people who are familiar with Python. tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@cclauss @nodejs-github-bot @gengjiawen @richardlau @anonrig @MoLow @Trott