Start enforcing bugbear rules (B) #1602
Conversation
|
Please uncomment this line to enable the bugbear rules: filesystem_spec/pyproject.toml Line 152 in 0bb3f26 |
|
I believe going piecemeal is the best approach, as you have been doing |
|
What I can do is enable |
fsspec/caching.py
Outdated
| @@ -574,7 +574,7 @@ def __init__( | |||
| blocksize: int, | |||
| fetcher: Fetcher, | |||
| size: int, | |||
| data: dict[tuple[int, int], bytes] = {}, | |||
| data: dict[tuple[int, int], bytes] = None, | |||
There was a problem hiding this comment.
The type hint is now wrong. It could be made into a class attribute hint instead...
There was a problem hiding this comment.
I've added Optional[] for consistency with the other arguments, which all have a type hint.
fsspec/tests/test_utils.py
Outdated
| @@ -381,7 +381,7 @@ def client(self): | |||
| return mock | |||
|
|
|||
| def func_2(self): | |||
| assert False, "have to overwrite this" | |||
| assert False, "have to overwrite this" # noqa: B011 | |||
There was a problem hiding this comment.
Or perhaps disable this rule in all tests?
There was a problem hiding this comment.
This is certainly a special case, it's not normal to assert False! On second thoughts, I'm OK with the original noqa too.
There was a problem hiding this comment.
I changed to raise AssertionError in the mean time. Is that OK?
There was a problem hiding this comment.
In the end, I think both are fine :)
DimitriPapadopoulos
force-pushed
the
B
branch
4 times, most recently
from
164564b to
62ad8e0
Compare
|
I see you added additional rules one by one - is the process now finished? |
|
Yes, I'm finished in the near term, the remaining |
B015 Pointless comparison. Did you mean to assign a value? Otherwise, prepend `assert` or remove it.
B033 Sets should not contain duplicate item `"..."`
B006 Do not use mutable data structures for argument defaults
B018 Found useless expression. Either assign it to a variable or remove it.
B020 Loop control variable overrides iterable it iterates
B034 `re.split` should pass `maxsplit` and `flags` as keyword arguments
to avoid confusion due to unintuitive argument positions
B011 Do not `assert False` (`python -O` removes these calls),
raise `AssertionError()`
For now, ignore these: - B007 - B019 - B026 - B028 - B904
| @@ -139,7 +139,7 @@ def _fetch_range(self, start, end): | |||
| try: | |||
| import snappy | |||
|
|
|||
| snappy.compress | |||
| snappy.compress(b"") | |||
There was a problem hiding this comment.
Wait, this one isn't quite right, it's an attribute look up that could be an assert statement I suppose
There was a problem hiding this comment.
Indeed, I guess the idea of @martindurant was that the compress(b"") call cannot fail; any error has to be an attribute look up. I can revert to the pure attribute look up with # noqa - I agree it makes the intent clearer.
There was a problem hiding this comment.
An assert in regular code is not recommended, since they can be turned off for the whole interpreter session. I think the function call is fine, or if we wanted a different function call to appease linting, we could do a getattr.
Background: python-snappy is not the only installable to provide a package called snappy (not my fault!), so this is a way to differentiate. Actually, snappy-compressing whole files (as opposed to buffers within files) is super-rare, so we could perhaps even remove this, rather than increase import time by checking this. We should probably not be importing anything at import time!
In [1]: import fsspec
In [2]: import sys
In [3]: "snappy" in sys.modules
Out[3]: True
I intend to merge this PR as is; @DimitriPapadopoulos or @Skylion007 , you are most welcome to make a PR to defer imports in the fsspec.compression module.
No description provided.