-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-55266][INFRA] Add pre-commit hooks for format/lint #54048
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
Conversation
JIRA Issue Information=== Improvement SPARK-55266 === This comment was automatically generated by GitHub Actions |
|
I think we should provide a |
|
Developers can already run linters locally. This PR is to run it "automatically" before each commit. |
|
I see, does get this after reinstall |
|
|
||
| - id: ruff | ||
| name: Lint Python code with Ruff | ||
| entry: ./dev/lint-python --ruff |
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.
ok, we don't check mypy which is pretty slow (and flaky in my local).
Then LGTM
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 think it is slow because mypy is checking all the dependencies. can we let it not to do it?
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.
mypy is slow, especially without cache. It's pure python. astral and meta are both working on a rust-based type checker which are significantly faster than mypy, but neither of them is stable enough for production use.
|
Yeah mypy is much slower and not as consistent across environments. We only do black + ruff for pre-commit. Are there a lot of error reports for your mypy? I've updated some requirements while I'm fixing mypy, |
I believe you are running lint on 3.12. The lint image is 3.11. |
|
Love this feature!
BTW, updating lint image to 3.12 here in #54042. |
I have been using 3.13 for some time :) |
|
Merged to master. |
What changes were proposed in this pull request?
Add pre-commit yaml file to allow users optionally enable pre-commit hook.
Why are the changes needed?
It's common that users forget to check lint/format and have to wait for 2 hours to realize it. This prevents that situation. Also python reformatter and lint is sub-second so it won't impact user too much.
This is also optional. User won't feel it without explicitly installing
pre-commitDoes this PR introduce any user-facing change?
No
How was this patch tested?
Local pre-commit worked.
Was this patch authored or co-authored using generative AI tooling?
No