-
Notifications
You must be signed in to change notification settings - Fork 579
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.
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
Migrate formatting and linting to ruff #3816
Conversation
…ve old qa comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to do it after bumping black but thanks for beating me to it :)
@@ -660,7 +649,14 @@ def format_args(args): | |||
) | |||
runsubprocess( | |||
args.dry_run, | |||
("isort", "--settings-path", f"{root_dir}/.isort.cfg", "--profile", "black", "."), | |||
( | |||
"isort", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you still keep isort and black here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops i missed this thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in new PR
Sorry didn't realize you were working on it! I can give you permission to push to the PR if you'd like to collaborate here Fyi I should probably split this into two PRs (or at least commits) for easier reviewing:
I think the second will be more useful to actually scrutinize for missing coverage |
Don't worry, it was just a plan :)
Separate commits for formatting and linting will be appreciated, thanks! |
Closing in favor of two PRs. First one here #3822 |
Description
Migrates pylint, flake8, black, and isort to ruff. Ruff is very fast and has complete feature parity with all of the above except some pylint features. Ruff does not use the python environment at all, so it no longer requires any deps to be installed in the virtualenv.
Dropping pylint is definitely worth discusing, because we don't run mypy on all of our sources. See here for a list of tradeoffs and recommendations. I think this is a decent trade off given how clunky, difficult to use (e.g. #3814), and slow pylint is. We should try to enable mypy on the rest of our sources imo.
Fixes #3260 and numerous complaints about the painfully slow lint/formatting process
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Does This PR Require a Contrib Repo Change?
Answer the following question based on these examples of changes that would require a Contrib Repo Change:
The OTel specification has changed which prompted this PR to update the method interfaces of
opentelemetry-api/
oropentelemetry-sdk/
The method interfaces of
test/util
have changedScripts in
scripts/
that were copied over to the Contrib repo have changedConfiguration files that were copied over to the Contrib repo have changed (when consistency between repositories is applicable) such as in
pyproject.toml
isort.cfg
.flake8
When a new
.github/CODEOWNER
is addedMajor changes to project information, such as in:
README.md
CONTRIBUTING.md
Yes. - Link to PR:
No.
Checklist: