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

Use ruff for linting #347

Merged
merged 7 commits into from
Jan 19, 2024
Merged

Use ruff for linting #347

merged 7 commits into from
Jan 19, 2024

Conversation

tqa236
Copy link
Contributor

@tqa236 tqa236 commented Jan 14, 2024

Description

Hello,

This MR configures ruff, an extremely fast Python linter and code formatter to vulture. ruff can potentially replace all the current linting and formatting tools, namely black, flake8 and pyupgrade.

The current config in pyproject.toml is the default one, I put it there for ease of your review and customization.

Let me know if this change is welcome. I can make further adjustments to the config to suit vulture style if needed.

Checklist:

  • I have updated the documentation in the README.md file or my changes don't require an update.
  • I have added an entry in CHANGELOG.md.
  • I have added or adapted tests to cover my changes.
  • I have run tox -e fix-style to format my code and checked the result with tox -e style.

@jendrikseipp
Copy link
Owner

Thanks for the PR! I like the proposed changes. In addition, can you make sure that we enable the same ruff tests as were enabled previously via the other tools? And then remove the other tools?

@tqa236
Copy link
Contributor Author

tqa236 commented Jan 15, 2024

Hi @jendrikseipp, this PR is ready for review.

There are a few things to consider:

  • ruff can replace all existing linting packages. I have configured it to the same rules and removed the redundant packages.
  • ruff can even replace black with as a main formatter (some big packages like pandas already migrated, ref). I can do this in the next PR if you think vulture can adopt this change. This is the documentation about the known deviations between ruff and black.
  • I keep the current tox -e style and tox -e fix-style as the main entry points for now, but they can also be replaced completely with pre-commit. I do that in the next PR if you're ok with the deprecations of these tox commands.

Copy link
Owner

@jendrikseipp jendrikseipp left a comment

Choose a reason for hiding this comment

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

Thanks for revising the PR! I have a few more nitpicks, but I like where this is going :-)

I see the differences between ruff and black as an advantage for ruff. So if you like, it would be great if you could change the formatter to ruff in a separate PR later.

Also, feel free to replace the "tox -e style" and "tox -e fix-style" commands with pre-commit in another PR.

Great stuff!

pyproject.toml Show resolved Hide resolved
pyproject.toml Outdated
target-version = "py38"

[tool.ruff.lint]
select = ["E4", "E7", "E9", "F", "B", "C4", "UP", "W", "C901"]
Copy link
Owner

Choose a reason for hiding this comment

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

Please add a comment telling which checks are enabled by ruff by default.

Copy link
Owner

Choose a reason for hiding this comment

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

Also, please add short comments on what these letters mean. For both of these changes, it's probably best to convert the list to have one entry per line, with a brief comment next to it.

pyproject.toml Outdated
select = ["E4", "E7", "E9", "F", "B", "C4", "UP", "W", "C901"]
ignore = [
# C408: unnecessary dict call
"C408",
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
"C408",
"C408", # unnecessary dict call

(use one line)

vulture/core.py Outdated
@@ -256,7 +256,7 @@ def handle_syntax_error(e):
except SyntaxError as err:
handle_syntax_error(err)

def scavenge(self, paths, exclude=None):
def scavenge(self, paths, exclude=None): # noqa: C901
Copy link
Owner

Choose a reason for hiding this comment

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

Let's just remove C901 from the list of selected tests instead.

tox.ini Outdated

[testenv:fix-style]
basepython = python3
deps =
black==22.3.0
pyupgrade==2.28.0
ruff==0.1.13
allowlist_externals =
bash
Copy link
Owner

Choose a reason for hiding this comment

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

The bash special case is obsolete now. Same in the "style" part above.

runs-on: ubuntu-latest
steps:
- uses: actions/[email protected]
- uses: actions/setup-python@v5
Copy link
Owner

Choose a reason for hiding this comment

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

Is it necessary to set up a Python version here? The ubuntu-latest image already comes with Python installed.

@tqa236
Copy link
Contributor Author

tqa236 commented Jan 19, 2024

@jendrikseipp I updated the code according to your review. Can you take a look again, please? The list of patterns ignored by ruff now is a fuse of .gitignore and the one ignored by black.

Noted about the future improvement of ruff format and pre-commit, I can tackle them in follow-up PRs

@tqa236 tqa236 mentioned this pull request Jan 19, 2024
4 tasks
@jendrikseipp jendrikseipp merged commit aa42062 into jendrikseipp:main Jan 19, 2024
16 checks passed
@jendrikseipp
Copy link
Owner

Great, thanks!

In an upcoming PR, please add a changelog entry for this and the other planned changes. And feel free to add your name or nick to the changelog entry.

@tqa236 tqa236 deleted the configure-ruff branch March 19, 2024 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants