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.

Sign up for GitHub

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

Jump to bottom

[lint] replaces black formatting with ruff #14132

Merged
merged 13 commits into from
Jan 11, 2024
Merged

[lint] replaces black formatting with ruff #14132

merged 13 commits into from
Jan 11, 2024

Conversation

iris-garden
Copy link
Contributor

@iris-garden iris-garden commented Jan 9, 2024 •
edited
Loading

ruff is faster than black when formatting Python files, so this change replaces the formatting checks for our files with calls to ruff instead of black, and runs ruff format on all files that would cause those checks to fail.

@iris-garden iris-garden mentioned this pull request Jan 9, 2024
Merged
@iris-garden iris-garden mentioned this pull request Jan 9, 2024
Closed
@iris-garden
Copy link
Contributor Author

oh wait, okay, i'm gonna see if there's an equivalent to skip-string-normalization for ruff, since i think that might be literally all of these automatic changes haha

github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 9, 2024
View reviewed changes
batch/batch/driver/main.py Fixed Show fixed Hide fixed
batch/batch/cloud/azure/worker/worker_api.py Fixed Show fixed Hide fixed
ci/ci/ci.py Fixed Show fixed Hide fixed
ci/create_database.py Fixed Show fixed Hide fixed
batch/batch/driver/job.py Fixed Show fixed Hide fixed
ci/create_database.py Fixed Show fixed Hide fixed
@iris-garden iris-garden force-pushed the lint/ruff-replace-black-format branch from 97b15b3 to 44d9c7e Compare January 9, 2024 19:15
@iris-garden
Copy link
Contributor Author

iris-garden commented Jan 9, 2024 •
edited
Loading

okay, enabled the quote-style = "preserve" flag, now this should be actually ready for a look! (there are still changes to quotes around some docstrings, but there's not a way to disable this for ruff currently)

@daniel-goldstein
Copy link
Contributor

daniel-goldstein commented Jan 9, 2024 •
edited
Loading

Interesting, PEP 8 does have an opinion about triple-quoted strings. FWIW, I'm happy to go stricter in the future and allow string normalization and all that (assuming other folks are on board), but I appreciate you aligning the configs for this PR!

daniel-goldstein
daniel-goldstein previously requested changes Jan 9, 2024
View reviewed changes
Copy link
Contributor

@daniel-goldstein daniel-goldstein left a comment

Choose a reason for hiding this comment

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

This looks great! One last request and I'll approve: could you remove black from the dev requirements and replace the pre-commit hook?

hail/python/hail/experimental/datasets.py Outdated
f' dataset {name}.\n'
f'Available platforms: {clouds}.')
raise ValueError(
f'Cloud platform {repr(cloud)} not available for' f' dataset {name}.\n' f'Available platforms: {clouds}.'
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is a little unfortunate. Let's combine them into one f-string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

@iris-garden
Copy link
Contributor Author

iris-garden commented Jan 9, 2024 •
edited
Loading

fixed up the dev requirements, and updated the pre-commit hook, and in the process realized i had only run ruff format on files that are checked by make check-all, so also ran it at the root of the repo, as the hook will run there

EDIT: i misunderstood how the pre-commit hooks work, and apparently it only runs them on the files you have changed, but i think it's still worth it to format across the repo, not just in the places we check with make check-all; and also, i personally was a grump and never used the pre-commit hooks, but now that i've had to test them out for this, i have seen the light ⚡

also i personally would love if our quotes were consistent as well! i'll ask on zulip how ppl feel about that

github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 9, 2024
View reviewed changes
devbin/rotate_keys.py Dismissed Show dismissed Hide dismissed
@danking danking merged commit fa2ef0f into hail-is:main Jan 11, 2024
8 checks passed
danking pushed a commit that referenced this pull request Jan 16, 2024
@iris-garden
[lint] ignores black->ruff formatting commit (#14133)
ce58fa9 
See #14132.
chrisvittal pushed a commit to chrisvittal/hail that referenced this pull request Jul 10, 2024
@iris-garden @chrisvittal
[lint] ignores black->ruff formatting commit (hail-is#14133)
2d0b8eb 
See hail-is#14132.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daniel-goldstein daniel-goldstein daniel-goldstein approved these changes

Assignees

@daniel-goldstein daniel-goldstein

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@iris-garden @daniel-goldstein @danking