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

feat: migrate to ruff for linting and formatting #2530

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

banteg
Copy link
Contributor

@banteg banteg commented Feb 26, 2025

Summary

  • Replace black, isort, flake8, and various flake8 plugins with ruff
  • Configure ruff to maintain the same coding style and linting rules
  • Update pre-commit hooks to use ruff for formatting and linting

Test plan

  • The project should build and pass all tests
  • Code formatting should be consistent with previous style
  • No functional changes were made, only tooling updates

🤖 Generated with Claude Code

@banteg
Copy link
Contributor Author

banteg commented Feb 26, 2025

lint fails are expected because the linters are removed and replaced with a different pipeline, both in pyproject and ci

Copy link
Member

@antazoey antazoey left a comment

Choose a reason for hiding this comment

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

It looks like ruff found a lot of helpful stuff the other tools missed which is pretty cool. I don't like the if / elif change though.

@@ -43,4 +33,4 @@ repos:
additional_dependencies: [mdformat-gfm, mdformat-frontmatter, mdformat-pyproject]

default_language_version:
python: python3
python: python3
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
python: python3
python: python3


[tool.mdformat]
number = true
number = true
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
number = true
number = true

@@ -26,35 +26,34 @@ def __getattr__(name: str):
if name not in __all__:
raise AttributeError(name)

elif name == "reverts":
if name == "reverts":
Copy link
Member

Choose a reason for hiding this comment

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

I've always preferred elif in these cases in case you need to switch from returning here to building up a return (have had a rookie mistakes back in the day from that).

Can we disable this rule?

- Replace black, flake8, isort with unified ruff tooling
- Disable RET (flake8-return) and SIM (flake8-simplify) rules per maintainer feedback
- Remove some opinionated code style rewrites that would change if/elif patterns
- Maintain existing code style preferences for conditionals
@banteg
Copy link
Contributor Author

banteg commented Feb 26, 2025

i disabled the rules you don't want, but there is a bunch more to go through. im handing off this pr at this point, it's up to your prefernce which of the remaining rules you like and which you find annoying. you can tune the active rules in pyproject.toml.

you can find all the rules here https://docs.astral.sh/ruff/rules/

ruff check src --statistics
88	BLE001 	[ ] blind-except
85	PGH003 	[ ] blanket-type-ignore
66	RUF012 	[ ] mutable-class-default
18	C416   	[*] unnecessary-comprehension
17	B904   	[ ] raise-without-from-inside-except
16	C408   	[*] unnecessary-collection-call
 7	B007   	[*] unused-loop-control-variable
 6	ERA001 	[*] commented-out-code
 6	C417   	[*] unnecessary-map
 4	B028   	[*] no-explicit-stacklevel
 4	PIE810 	[*] multiple-starts-ends-with
 3	C414   	[*] unnecessary-double-cast-or-process
 2	B012   	[ ] jump-statement-in-finally
 2	TC003  	[*] typing-only-standard-library-import
 2	PTH123 	[ ] builtin-open
 2	UP028  	[*] yield-in-for-loop
 1	C401   	[*] unnecessary-generator-set
 1	C409   	[*] unnecessary-literal-within-tuple-call
 1	C419   	[*] unnecessary-comprehension-in-call
 1	PT003  	[*] pytest-extraneous-scope-function
 1	TC001  	[*] typing-only-first-party-import
 1	PTH101 	[ ] os-chmod
 1	PTH118 	[ ] os-path-join
 1	PLW0127	[ ] self-assigning-variable
 1	PLW1509	[ ] subprocess-popen-preexec-fn
 1	RUF005 	[ ] collection-literal-concatenation
 1	RUF015 	[*] unnecessary-iterable-allocation-for-first-element

- Replace black, isort, and flake8 with ruff check and ruff format
- Maintain same CI validation functionality with new tooling

🤖 Generated with Claude Code
Co-Authored-By: Claude <[email protected]>
- Replace black, isort, and flake8 with ruff check and ruff format in GitHub Actions
- Remove unnecessary Black configuration from pyproject.toml
- Complete migration from multiple linting tools to unified Ruff toolchain

🤖 Generated with Claude Code
Co-Authored-By: Claude <[email protected]>
@fubuloubu
Copy link
Member

I saw it raises for print but does it raise for breakpoint?

@banteg
Copy link
Contributor Author

banteg commented Feb 26, 2025

I saw it raises for print but does it raise for breakpoint?

i think it's the T100 rule

@banteg
Copy link
Contributor Author

banteg commented Feb 28, 2025

i went through all ruff rules with claude and updated the config to better fit the existing code style. this is what it had to say:

Our refined Ruff configuration successfully addresses all the linting issues across both the src and tests directories.

Here's a summary of the key aspects of our final configuration:

  1. Selected Rule Categories:
    - Core Python errors and style rules (E, F, W)
    - Import sorting (I)
    - Code quality with Bugbear (B)
    - Code simplification (SIM, C4)
    - Modern Python (UP)
    - Return statement improvements (RET)
    - Security (S)
    - Type checking (TCH)
  2. Key Ignored Rules:
    - Code style preferences that would require major refactoring
    - Rules that didn't match the project style (like naming conventions)
    - Security rules that had legitimate uses in this specific codebase
    - Common patterns in the codebase that might be flagged as issues
  3. Per-file Overrides:
    - Test files have relaxed rules for docstrings, line length, and assertions
    - Fixture files allow unused imports and variables
    - Integration test project files allow unused variables where needed

This configuration provides a good balance of code quality enforcement while respecting the existing patterns in the codebase. It can be applied incrementally, focusing first on the most critical issues before gradually addressing more stylistic concerns.

]

[tool.ruff.lint.pydocstyle]
convention = "google"
Copy link
Member

Choose a reason for hiding this comment

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

Oh sick

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.

3 participants