Add ruff pre-commit hook #1502
Add ruff pre-commit hook #1502
Conversation
|
Did it only check changed files? |
|
@martindurant Nope, I was just conservative with it so it exactly match flake8. I am about to add a bunch more rules and changes in a few seconds. |
|
@martindurant Okay, I enabled a bunch of new rules and added a ton of fixes. |
Skylion007
force-pushed
the
skylion007/add-ruff-precommit
branch
4 times, most recently
from
4e29445 to
410891c
Compare
|
This PR also only enables the linter, not the formatter. |
Skylion007
force-pushed
the
skylion007/add-ruff-precommit
branch
from
410891c to
a0d0115
Compare
| @@ -19,7 +15,7 @@ class AbstractCacheMapper(abc.ABC): | |||
| def __call__(self, path: str) -> str: | |||
| ... | |||
|
|
|||
| def __eq__(self, other: Any) -> bool: | |||
| def __eq__(self, other: object) -> bool: | |||
There was a problem hiding this comment.
So lon we've been told to the the opposite, no?
There was a problem hiding this comment.
https://docs.astral.sh/ruff/rules/any-eq-ne-annotation/ Happy to revert this rule too and disable if necessary, just let me know.
| @@ -251,7 +251,7 @@ async def _get_file( | |||
| if isfilelike(lpath): | |||
| outfile = lpath | |||
| else: | |||
| outfile = open(lpath, "wb") | |||
| outfile = open(lpath, "wb") # noqa: ASYNC101 | |||
There was a problem hiding this comment.
What would be the problem here?
There was a problem hiding this comment.
https://docs.astral.sh/ruff/rules/open-sleep-or-subprocess-in-async-function/ Not sure there is a good way around it without relying the aiofiles dependency though so I just suppressed it
There was a problem hiding this comment.
What a strange rule! Just about any normal function call blocks for some time, and open() ought to be fast. I don't think there's a big reason to switch to aiofiles right now, maybe later :)
There was a problem hiding this comment.
Yeah, I just left it because it seemed useful and we can noqa the nonsense suggestions like this. 👍
| @@ -123,6 +123,7 @@ def test_chmod(mount_local): | |||
| ["cp", str(mount_dir / "text"), str(mount_dir / "new")], | |||
| stdout=subprocess.PIPE, | |||
| stderr=subprocess.PIPE, | |||
| check=False, | |||
There was a problem hiding this comment.
https://docs.astral.sh/ruff/rules/subprocess-run-without-check/ Makes you think about whether you should enable to reraise an exception if the process fails (instead of having it fail silently). Happy to disable that if you prefer though.
There was a problem hiding this comment.
I suppose check=True is reasonable, then. In a test like this, we would otherwise have the failure a couple of lines later. I'm not really sure why we're using subprocess at all in this case.
There was a problem hiding this comment.
@martindurant Check=True made the test fail so I am just leaving it as is for now.
Co-authored-by: Martin Durant <[email protected]>
This reverts commit cd48c84.
|
@martindurant the one failure looks like a flake. |
|
I have yet to determine why SMB fails to start sometimes |
This reverts commit d0ceefb.
|
@Skylion007 - merging this now. I may ping you if future PRs show odd ruff failures I don't understand :). |
Closes #1501
I mostly only enabled rules that had autofixes available (and were therefore easy to enable, did not change large portions of the codebase, and are rather uncontroversial). Many of these changes are semantically identical but run more efficiently in Python (set generator -> set comprehension). Other changes make the typing more correct with the flake8-PYI linter and so on.
Good rules to enable in future PRs: