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.

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

Introducing ruff as linter and formatter (basic setup) #4223

Merged
merged 5 commits into from
Oct 15, 2024

Conversation

emdneto
Copy link
Member

@emdneto emdneto commented Oct 13, 2024

Description

This PR swaps out black, isort, and flake8 for ruff, which handles all three in one go! 🎉 Based on this #3260 (comment) it seems there's a general agreement we can adopt ruff.

Closes #3895 #3260
Superseds #3822

Why this change?

We had some issues regarding slow lint in CI and locally (#3887, #3847, ), which required us to use a strategy of linting each package individually, leading to multiple lint commands in tox.ini and several lint jobs in CI. At this moment, lint is very slow to run locally (even by using the tox command)

The main benefit I see here is that we are replacing 3 tools and removing a lot of lint commands from tox.ini. Also ruff is very fast and the configuration is pretty easy to follow.

Note

Right now, ruff has different rules than Pylint. So the question is: Can we take the risk of not using Pylint anymore and relying only on Ruff? In this PR, I'll keep Pylint CI jobs until we decide. If we agree to rely only on Ruff, we can drop all Pylint CI jobs.

What’s changed?

  • Removed configs for black, isort, and flake8.
  • Added basic ruff config to pyproject.toml
  • Updated the pre-commit configuration to use ruff / also updated CONTRIBUTING.md with new instructions
  • Added ruff checks to CI (using pre-commit)

@emdneto emdneto requested a review from a team as a code owner October 13, 2024 01:18
@emdneto emdneto added the build & infra Issues related to build & infrastructure. label Oct 13, 2024
@emdneto emdneto changed the title Introducing ruff as linter and formatter Introducing ruff as linter and formatter (basic setup) Oct 13, 2024
@emdneto
Copy link
Member Author

emdneto commented Oct 13, 2024

Note to reviewers

Since there are a lot of formatting changes, here are the actual changed files 7a6052e:

.flake8
.github/workflows/misc_0.yml
.isort.cfg
.pre-commit-config.yaml
CONTRIBUTING.md
dev-requirements.txt
lint-requirements.txt
pyproject.toml
scripts/semconv/generate.sh
tox.ini

For everyone who wants to see it working:

  1. Checkout the branch
  2. tox -e ruff

Also, the idea is that we add more rules to ruff linter in separate PRs since there are a lot of fixes to do (maybe create a separate issue for that) . e.g.,

select = [
  "I",   # isort
  "F",   # pyflakes
  "E",   # pycodestyle errors
  "W",   # pycodestyle warnings
  "N",   # pep8-naming
  "C90", # mccabe
  "RUF", # Ruff-specific rules
  "UP",  # pyupgrade
  "ERA", # eradicate
  "PLC", # pylint - convention
  "PLE", # pylint - error
  "PLW", # pylint - warning
  "A",   # flake8-builtins
  "B",   # flake8-bugbear
  "BLE", # flake8-blind-except
  "C4",  # flake8-comprehensions
  "Q",   # flake8-quotes
  "G",   # flake8-logging-format
  "ICN", # flake8-import-conventions
  "ISC", # flake8-implicit-str-concat
  "PIE", # flake8-pie
  "PTH", # flake8-use-pathlib
  "RET", # flake8-return
  "S",   # flake8-bandit
  "SIM", # flake8-simplify
  "T10", # flake8-debugger
  "T20", # flake8-print
  "TCH", # flake8-type-checking
  "TID", # flake8-tidy-imports
]

@xrmx
Copy link
Contributor

xrmx commented Oct 14, 2024

Love this but I am not sure out of the box ruff will sort imports like isort would do (system, third party, first party sections).

@emdneto
Copy link
Member Author

emdneto commented Oct 14, 2024

Love this but I am not sure out of the box ruff will sort imports like isort would do (system, third party, first party sections).

There's a configuration for ruff.lint.isort that is section-order. By default is: ["future", "standard-library", "third-party", "first-party", "local-folder"] but we can set the order we want. How does this sound to you?

@xrmx
Copy link
Contributor

xrmx commented Oct 14, 2024

Love this but I am not sure out of the box ruff will sort imports like isort would do (system, third party, first party sections).

There's a configuration for ruff.lint.isort that is section-order. By default is: ["future", "standard-library", "third-party", "first-party", "local-folder"] but we can set the order we want. How does this sound to you

With ruff 0.6.9 if I run ruff format over:

from opentelementry.trace import foo
import os

It will not change anything, I may be missing something in this PR that changes the behavior though

@emdneto
Copy link
Member Author

emdneto commented Oct 14, 2024

With ruff 0.6.9 if I run ruff format over:

from opentelementry.trace import foo
import os

It will not change anything, I may be missing something in this PR that changes the behavior though

Ohh I see. The thing is, in ruff, the isort runs at the lint stage, so you need to run the check instead of the format (It was a little bit confusing to me too)

ruff check test.py --fix

In the pyproject.toml file, there's a section for ruff.lint.isort that configures some parameters as well.

import psutil
from opentelementry.trace import foo
import os

run ruff check test.py --fix

becomes:

import os

import psutil
from opentelementry.trace import foo

Signed-off-by: emdneto <[email protected]>

remove unecessary lint stuff

Signed-off-by: emdneto <[email protected]>

update pyproject.toml and pre-commit to auto fix lint

Signed-off-by: emdneto <[email protected]>

add parameter --show-fixes to pre-commit

Signed-off-by: emdneto <[email protected]>

some fixes to generate.sh in semconv

Signed-off-by: emdneto <[email protected]>
@emdneto emdneto added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Oct 14, 2024
Copy link
Contributor

@xrmx xrmx left a comment

Choose a reason for hiding this comment

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

This is great, thank you!

Copy link
Contributor

@tammy-baylis-swi tammy-baylis-swi left a comment

Choose a reason for hiding this comment

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

Lgtm, esp how much shorter the tox.ini is now!

One thought. This question on the other PR seems to be why we didn't merge it.

Adding "do not merge label" until we decide for sure on Ruff replacing flake8 and pylint as well.

I think so, with the basic [tool.ruff.lint] for now and more rules later as you mentioned.

Copy link
Contributor

@lzchen lzchen left a comment

Choose a reason for hiding this comment

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

@emdneto

This looks awesome! QQ: Any idea what is going on with the spellcheck env?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build & infra Issues related to build & infrastructure. Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate better tools for linting
4 participants