-
Notifications
You must be signed in to change notification settings - Fork 2k
Description
Using ruff 0.3.4 and attempting to adopt bugbear (B) checks in the keyring project, I'm encountering what appear to be false positives:
keyring main @ ruff check --select B018 --fix --unsafe-fixes
keyring/backend.py:71:13: B018 Found useless expression. Either assign it to a variable or remove it.
keyring/backends/SecretService.py:35:13: B018 Found useless expression. Either assign it to a variable or remove it.
keyring/backends/Windows.py:14:9: B018 Found useless expression. Either assign it to a variable or remove it.
keyring/backends/Windows.py:21:9: B018 Found useless expression. Either assign it to a variable or remove it.
keyring/core.py:137:5: B018 Found useless expression. Either assign it to a variable or remove it.
keyring/testing/backend.py:177:13: B018 Found useless expression. Either assign it to a variable or remove it.
(there was one other prior to jaraco/keyring@7881db6)
Looking at keyring.backend:71, it's apparent that the "useless expression" is cls.priority. The protocol for keyring, however, is for a backend to raise an exception in the .priority access if the backend is not viable at all.
Looking at keyring.backends.Windows:14, the purpose of the attribute access is explicitly documented (force demand import to import the module).
The advice is wrong. In particular, the expression isn't useless (the application would fail without the side effect of accessing the property), and the guidance is misleading (assigning to a variable would introduce more lint and removing it would break the the behavior).
I can imagine one possible workaround is to replace direct attribute access with a getattr call:
diff --git a/keyring/backend.py b/keyring/backend.py
index a21418f..e082fbe 100644
--- a/keyring/backend.py
+++ b/keyring/backend.py
@@ -68,7 +68,7 @@ class KeyringBackend(metaclass=KeyringBackendMeta):
@properties.classproperty
def viable(cls):
with errors.ExceptionRaisedContext() as exc:
- cls.priority
+ getattr(cls, 'priority')
return not exc
@classmethod
diff --git a/keyring/backends/Windows.py b/keyring/backends/Windows.py
index 7208ed1..5d057ec 100644
--- a/keyring/backends/Windows.py
+++ b/keyring/backends/Windows.py
@@ -11,7 +11,7 @@ with ExceptionRaisedContext() as missing_deps:
from win32ctypes.pywin32 import pywintypes, win32cred
# force demand import to raise ImportError
- win32cred.__name__
+ getattr(win32cred, '__name__')
except ImportError:
# fallback to pywin32
import pywintypesHowever, this approach makes the code less readable and idiosyncratic (the canonical way to access an attribute is with dot notation), and making the change introduces B009 errors:
keyring main @ ruff check --select B009
keyring/backend.py:71:13: B009 [*] Do not call `getattr` with a constant attribute value. It is not any safer than normal property access.
keyring/backends/Windows.py:14:9: B009 [*] Do not call `getattr` with a constant attribute value. It is not any safer than normal property access.
Another workaround could be to disable B018 checks for all projects (or possibly just this project; I'm not yet sure how much this issue is isolated to keyring). That would be somewhat undesirable. It would be nice to get an error for failing to call a method (e.g. self._init).
The workaround I'm leaning toward is to put noqa: B018 on each of these expressions, but that feels messy.
Since I'm not confident that ruff could readily distinguish between a property access that has a side-effect and a useless property access, I wonder if the documentation should at least capture this scenario and make a recommendation on what to do in that case.