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

[chore] unify local and pre-commit ruff configuration #23501

Merged

Conversation

danielgafni
Copy link
Contributor

@danielgafni danielgafni commented Aug 8, 2024

Summary & Motivation

This PR makes pre-commit use the same ruff settings & executable as the local environment (e.g. when run via make ruff).

Should we add a buildkite job for pre-commit? Currently it's not being tested in CI.

Context: https://dagsterlabs.slack.com/archives/C03A0D72A6T/p1723065934987029

How I Tested These Changes

pre-commit can be enabled and doesn't produce any changes when run


P.S. I had to add a few = None placeholders in some of the notebooks since previously these expressions lacked any assignment and were invalid (ruff was failing completely on these files)

Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @danielgafni and the rest of your teammates on Graphite Graphite

@danielgafni danielgafni marked this pull request as ready for review August 8, 2024 08:10
@danielgafni danielgafni requested a review from PedramNavid August 8, 2024 08:11
@danielgafni danielgafni changed the title unify local and pre-commit ruff configuration [chore] unify local and pre-commit ruff configuration Aug 8, 2024
@danielgafni danielgafni force-pushed the 08-08-unify_local_and_pre-commit_ruff_configuration branch from 0907f4c to 4f07ef5 Compare August 8, 2024 08:21
@danielgafni danielgafni requested a review from cmpadden August 8, 2024 08:21
@cmpadden cmpadden requested a review from alangenfeld August 8, 2024 13:27
@alangenfeld alangenfeld requested review from rexledesma and removed request for alangenfeld August 8, 2024 16:24
Copy link
Contributor

@rexledesma rexledesma left a comment

Choose a reason for hiding this comment

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

I don't use pre-commit anymore so I don't really have a strong opinion here. We could also consider just passing the pyproject.toml directly: astral-sh/ruff-pre-commit#54 (comment).

@danielgafni
Copy link
Contributor Author

Should we get more approvals here or is it ok to merge?

@danielgafni
Copy link
Contributor Author

danielgafni commented Aug 19, 2024

@rexledesma it will be picked up by ruff when run from the local hook

@danielgafni danielgafni force-pushed the 08-08-unify_local_and_pre-commit_ruff_configuration branch from 4f07ef5 to 5d8d375 Compare August 19, 2024 23:00

Verified

This commit was signed with the committer’s verified signature.
rhennigan Rick Hennigan
@danielgafni danielgafni force-pushed the 08-08-unify_local_and_pre-commit_ruff_configuration branch from 5d8d375 to dc9b267 Compare August 19, 2024 23:02
@danielgafni danielgafni merged commit 98b39b8 into master Aug 19, 2024
1 of 2 checks passed
@danielgafni danielgafni deleted the 08-08-unify_local_and_pre-commit_ruff_configuration branch August 19, 2024 23:22
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.

None yet

3 participants