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

Add ruff to pre-commit and run against all files #815

Merged
merged 1 commit into from
Sep 22, 2023

Conversation

droctothorpe
Copy link
Contributor

@droctothorpe droctothorpe commented Sep 11, 2023

This PR adds ruff to the pre-commit hooks and resolves linting violations listed by an execution of pre-commit run --all-files. A few errors are ignored, namely:

  • F401 since it complains about unused imports in all of the __init__.py files.
  • E402 since the auto-generated conf.py begins with comments rather than imports.
  • E501 since there are so many violations (around 70), particularly in controller.py. If this is enforced, it might make sense to handle the remediation in a discrete PR.

Issue: #814

Copy link
Member

@jacobtomlinson jacobtomlinson 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 this.

It looks like this doesn't take into account the existing flake8 config, so that will need to be ported over. I expect that will at least solve the line length issues.

[flake8]
# References:
# https://flake8.readthedocs.io/en/latest/user/configuration.html
# https://flake8.readthedocs.io/en/latest/user/error-codes.html
# Note: there cannot be spaces after comma's here
exclude = __init__.py,versioneer.py,distributed/_concurrent_futures_thread.py
ignore =
E20, # Extra space in brackets
E231,E241, # Multiple spaces around ","
E26, # Comments
E4, # Import formatting
E721, # Comparing types instead of isinstance
E731, # Assigning lambda expression
E121, # continuation line under-indented for hanging indent
E126, # continuation line over-indented for hanging indent
E127, # continuation line over-indented for visual indent
E128, # E128 continuation line under-indented for visual indent
E702, # multiple statements on one line (semicolon)
W503, # line break before binary operator
E129, # visually indented line with same indent as next logical line
E116, # unexpected indentation
F811, # redefinition of unused 'loop' from line 10
F841, # local variable is assigned to but never used
E741 # Ambiguous variable names
W504, # line break after binary operator
max-line-length = 120

Also for the unused imports I would add #noqa comments in the file instead of disabling the check.

Also it looks like the import sorting has introduced a circular import and is causing CI to fail.

Copy link
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

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

Nice. @jacobtomlinson I'd be curious to hear what the ruff experience is like here at some point. Might be worth doing something similar in other projects if you think it improves the developer experience (xref dask/dask#10196)

@droctothorpe
Copy link
Contributor Author

Congrats on being on the front page of HN, @jacobtomlinson!

@droctothorpe
Copy link
Contributor Author

Checking in on this (no pressure). Happy to break it up into two separate PRs if that helps, one that modifies the config, one that runs against all files.

Copy link
Member

@jacobtomlinson jacobtomlinson 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 the nudge, this generally looks great.

I'm always kinda sad about how pytest get mixed into the list of imports, I usually prefer it to be the 1st import line in a test file with a blank line below it. But that's the point of these tools I guess to remove these personal preferences in favour of consistency.

@jacobtomlinson jacobtomlinson merged commit 30234e3 into dask:main Sep 22, 2023
18 checks passed
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.

3 participants