Skip to content

Conversation

@gaogaotiantian
Copy link
Contributor

What changes were proposed in this pull request?

Add ruff as an option to lint our code.

Why are the changes needed?

Our pinned flake8 version is just too old - it can't even run on 3.12+. We can upgrade flake8 version but I think gradually switch to ruff is a better options. The main reason is ruff is much much faster than flake8. ruff returns the result almost immediately (ms-level) on whole spark repo - which means we can even hook it in the pre-commit in the future.

It is surprisingly compatible with flake8 - there's almost no code change needed (with two extra ignored lint types which we can fix in the future). Everything it finds is a real issue instead of a different taste.

ruff can also serve as a black-compatible formatter which means we can probably ditch both flake8 and black in the future.

For now we only enable this option - it's not hooked into any CI or full ./dev/lint-python. However, I think we should do that soon.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Local lint test passed.

Was this patch authored or co-authored using generative AI tooling?

No

@Yicong-Huang
Copy link
Contributor

Tried ruff and it is super quick! in this case shall we add ruff to requirements.txt and pin the version?

@gaogaotiantian
Copy link
Contributor Author

Well for this PR I think I'd just focus on making it an option. Then I'll start a PR to include this in our CI and probably replace flake8 by default (leaving flake8 an option if users explicitly ask for it). In that PR we will add the requirements.

Copy link
Contributor

@allisonwang-db allisonwang-db left a comment

Choose a reason for hiding this comment

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

I like ruff linter. But should we pick one linter to use instead of adding it as an option?

@gaogaotiantian
Copy link
Contributor Author

We definitely should. Eventually we will use ruff as the single source of truth. However, we need a phase to do this switch. We need to confirm ruff is working properly, and leave flake8 as an option for a while for deprecation. Sudden switch to ruff could potentially break people's workflow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants