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 pre-commit hook #1502

Merged
merged 7 commits into from
Jan 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,13 @@ repos:
rev: 22.3.0
hooks:
- id: black
- repo: https://github.com/astral-sh/ruff-pre-commit
# Ruff version.
rev: v0.1.13
hooks:
# Run the linter.
- id: ruff
args: [ --fix ]
- repo: https://github.com/PyCQA/flake8
rev: 5.0.4
hooks:
Expand Down
4 changes: 0 additions & 4 deletions fsspec/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,8 @@ class BlocksizeMismatchError(ValueError):
written with
"""

...


class FSTimeoutError(asyncio.TimeoutError):
"""
Raised when a fsspec function timed out occurs
"""

...
5 changes: 3 additions & 2 deletions fsspec/gui.py
Original file line number Diff line number Diff line change
Expand Up @@ -153,8 +153,9 @@ def _emit(self, sig, value=None):
break
except Exception as e:
logger.exception(
"Exception (%s) while executing callback for signal: %s"
"" % (e, sig)
"Exception (%s) while executing callback for signal: %s",
e,
sig,
)

def show(self, threads=False):
Expand Down
8 changes: 2 additions & 6 deletions fsspec/implementations/cache_mapper.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,9 @@

import abc
import hashlib
from typing import TYPE_CHECKING

from fsspec.implementations.local import make_path_posix

if TYPE_CHECKING:
from typing import Any


class AbstractCacheMapper(abc.ABC):
"""Abstract super-class for mappers from remote URLs to local cached
Expand All @@ -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:
Copy link
Member

Choose a reason for hiding this comment

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

So lon we've been told to the the opposite, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://docs.astral.sh/ruff/rules/any-eq-ne-annotation/ Happy to revert this rule too and disable if necessary, just let me know.

# Identity only depends on class. When derived classes have attributes
# they will need to be included.
return isinstance(other, type(self))
Expand Down Expand Up @@ -56,7 +52,7 @@ def __call__(self, path: str) -> str:
else:
return prefix # No separator found, simple filename

def __eq__(self, other: Any) -> bool:
def __eq__(self, other: object) -> bool:
return super().__eq__(other) and self.directory_levels == other.directory_levels

def __hash__(self) -> int:
Expand Down
6 changes: 3 additions & 3 deletions fsspec/implementations/http.py
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ async def _get_file(
if isfilelike(lpath):
outfile = lpath
else:
outfile = open(lpath, "wb")
outfile = open(lpath, "wb") # noqa: ASYNC101
Copy link
Member

Choose a reason for hiding this comment

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

What would be the problem here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I just left it because it seemed useful and we can noqa the nonsense suggestions like this. 👍


try:
chunk = True
Expand Down Expand Up @@ -279,7 +279,7 @@ async def gen_chunks():
context = nullcontext(lpath)
use_seek = False # might not support seeking
else:
context = open(lpath, "rb")
context = open(lpath, "rb") # noqa: ASYNC101
use_seek = True

with context as f:
Expand Down Expand Up @@ -801,7 +801,7 @@ async def get_range(session, url, start, end, file=None, **kwargs):
async with r:
out = await r.read()
if file:
with open(file, "r+b") as f:
with open(file, "r+b") as f: # noqa: ASYNC101
f.seek(start)
f.write(out)
else:
Expand Down
2 changes: 1 addition & 1 deletion fsspec/implementations/tests/test_cached.py
Original file line number Diff line number Diff line change
Expand Up @@ -1022,7 +1022,7 @@ def test_multi_cache(protocol):
def test_multi_cat(protocol, ftp_writable):
host, port, user, pw = ftp_writable
fs = FTPFileSystem(host, port, user, pw)
for fn in {"/file0", "/file1"}:
for fn in ("/file0", "/file1"):
martindurant marked this conversation as resolved.
Show resolved Hide resolved
with fs.open(fn, "wb") as f:
f.write(b"hello")

Expand Down
2 changes: 1 addition & 1 deletion fsspec/implementations/tests/test_dirfs.py
Original file line number Diff line number Diff line change
Expand Up @@ -355,7 +355,7 @@ async def _walk(path, *args, **kwargs):

actual = []
async for entry in adirfs._walk("root", *ARGS, **KWARGS):
actual.append(entry)
actual.append(entry) # noqa: PERF402
assert actual == [("root", ["foo", "bar"], ["baz", "qux"])]
adirfs.fs._walk.assert_called_once_with(f"{PATH}/root", *ARGS, **KWARGS)

Expand Down
2 changes: 1 addition & 1 deletion fsspec/implementations/tests/test_http.py
Original file line number Diff line number Diff line change
Expand Up @@ -557,7 +557,7 @@ async def test_async_walk(server):
# No maxdepth
res = []
async for a in fs._walk(server + "/index"):
res.append(a)
res.append(a) # noqa: PERF402
assert res == [(server + "/index", [], ["realfile"])]

# maxdepth=0
Expand Down
4 changes: 2 additions & 2 deletions fsspec/implementations/tests/test_reference.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,9 @@ def test_nested_dirs_ls():
refs = {"a": "A", "B/C/b": "B", "B/C/d": "d", "B/_": "_"}
fs = fsspec.filesystem("reference", fo=refs)
assert len(fs.ls("")) == 2
assert set(e["name"] for e in fs.ls("")) == set(["a", "B"])
assert {e["name"] for e in fs.ls("")} == {"a", "B"}
assert len(fs.ls("B")) == 2
assert set(e["name"] for e in fs.ls("B")) == set(["B/C", "B/_"])
assert {e["name"] for e in fs.ls("B")} == {"B/C", "B/_"}


def test_info(server): # noqa: F811
Expand Down
2 changes: 1 addition & 1 deletion fsspec/implementations/tests/test_tar.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ def test_info():
assert lhs == expected

# Iterate over all files.
for f, v in archive_data.items():
for f in archive_data:
lhs = fs.info(f)

# Probe some specific fields of Tar archives.
Expand Down
2 changes: 1 addition & 1 deletion fsspec/implementations/tests/test_zip.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ def test_info():
fs = fsspec.filesystem("zip", fo=z)

# Iterate over all files.
for f, v in archive_data.items():
for f in archive_data:
lhs = fs.info(f)

# Probe some specific fields of Zip archives.
Expand Down
6 changes: 2 additions & 4 deletions fsspec/parquet.py
Original file line number Diff line number Diff line change
Expand Up @@ -131,10 +131,8 @@ def open_parquet_file(
cache_type="parts",
cache_options={
**options,
**{
"data": data.get(fn, {}),
"strict": strict,
},
"data": data.get(fn, {}),
"strict": strict,
},
**kwargs,
)
Expand Down
4 changes: 3 additions & 1 deletion fsspec/spec.py
Original file line number Diff line number Diff line change
Expand Up @@ -1398,7 +1398,9 @@ def to_json(self):
)
return json.dumps(
dict(
**{"cls": cls, "protocol": proto, "args": self.storage_args},
cls=cls,
protocol=proto,
args=self.storage_args,
**self.storage_options,
)
)
Expand Down
1 change: 1 addition & 0 deletions fsspec/tests/test_fuse.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Member

Choose a reason for hiding this comment

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

Why the extra arg

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@martindurant Check=True made the test fail so I am just leaving it as is for now.

)

assert cp.stderr == b""
Expand Down
1 change: 1 addition & 0 deletions fsspec/tests/test_spec.py
Original file line number Diff line number Diff line change
Expand Up @@ -1147,6 +1147,7 @@ def test_posix_tests_bash_stat(path, expected, glob_files_folder):
f"cd {glob_files_folder} && shopt -s globstar && stat -c %N {bash_path}",
],
capture_output=True,
check=False,
)
# Remove the last element always empty
bash_output = bash_output.stdout.decode("utf-8").replace("'", "").split("\n")[:-1]
Expand Down
53 changes: 53 additions & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,59 @@ exclude = '''
)
'''

[tool.ruff]
exclude = [
".tox",
"build",
"docs/source/conf.py",
"versioneer.py",
"fsspec/_version",
]
select = [
# fix noqas in fsspec/implementations/http.py
"ASYNC",
# "B", enable in later PR
"C4",
"G",
"E4",
"E7",
"E9",
"F",
"PERF",
"PLC",
"PLE",
"PLR1722",
"PLW1510",
"PLW3301",
"PIE800",
"PIE804",
"PIE807",
"PIE810",
# "PT", enable in later PR
"PYI",
"RUF006",
"SLOT",
"SIM101",
]
line-length = 88
ignore = [
# Assigning lambda expression
"E731",
# Ambiguous variable names
"E741",
# line break before binary operator
# uncomment when implemented in ruff
# "W503",
# whitespace before :
"E203",
# redefs
"F811",
# Fix these codes later
"G004",
"PERF203",
"PERF401",
]

[tool.pytest.ini_options]
# custom markers, need to be defined to avoid warnings
markers = [
Expand Down