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

[lint] uses ruff to format instead of black #14131

Closed
wants to merge 1 commit into from

Conversation

iris-garden
Copy link
Contributor

@iris-garden iris-garden commented Jan 9, 2024

As of #14119, we lint and format using black; this commit uses ruff instead, as it is faster and is intended to be a drop-in replacement for black. There are some rules that ruff cares about that black doesn't, which would require some dramatic reformats to the codebase, so for the time being, these are added to the list of rules to ignore, and can be added back in all at once or progressively later on.

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.

Thanks for doing this! I'm excited to have lightning fast linting and formatting for the whole codebase.

It's a little unclear to me though whether this is meant to tackle formatting and format-checking with ruff or adding hail to ruff's linting.

@@ -39,10 +39,8 @@ check-all: check-hail check-services

.PHONY: check-hail-fast
check-hail-fast:
ruff check hail/python/hail
ruff check hail/python/hailtop
ruff check hail
Copy link
Contributor

Choose a reason for hiding this comment

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

I think ruff check exclusively refers to the ruff linter. I think to check formatting you need ruff format --check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ohhhhhh thank you! i will take another look with this in mind

"PLC0132", # `TypeVar` name does not match assigned variable name
"PLE0307", # `__str__` does not return `str`
"PLC0414", # Import alias does not rename original package
"PLW0127", # Self-assignment of variable
Copy link
Contributor

Choose a reason for hiding this comment

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

I am all for these changes, but IIUC these are related to expanding the scope of ruff as a linter to include the hail directory, and not necessarily to do with replacing black as a formatter. I'm happy to review a big PR that includes linting fixes, just as long as it doesn't include auto-formatting changes.

@iris-garden
Copy link
Contributor Author

okay @daniel-goldstein #14132 replaces just the formatting/format checking with ruff, and i'll make another PR once that goes in to extend the ruff linting to the hail folder. thanks so much for your help on this! :)

@iris-garden iris-garden closed this Jan 9, 2024
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.

2 participants