fix(flake8_boolean_trap): add whitelist for dict methods #943
Conversation
This commit enables the skipping of FBT003 based on two conditions:
* function name must be explicitly whitelisted (for now, common dict
methods). In the future, more could be added based on user requests.
* argument in function call must be one of the first two. The idea is to
skip only things like `{}.get(False, "default")` or
`{}.get(key, True)`. Other method signature with a name in the
whitelist can still potentially generate FBT003.
fixes astral-sh#893
|
Upstream doesn’t raise FBT003 for method calls at all: |
src/flake8_boolean_trap/plugins.rs
Outdated
| /// | ||
| /// Conditions: | ||
| /// * function name must be explicitly whitelisted | ||
| /// * argument in function call must be one of the first two |
There was a problem hiding this comment.
Should it not be exactly the second argument, of two? Why "one of the first two"?
There was a problem hiding this comment.
booleans can be used as keys, too
There was a problem hiding this comment.
I guess, as an exception, the first argument of fromkeys shouldn't be boolean given that it's always a tuple. That's fine though.
src/flake8_boolean_trap/plugins.rs
Outdated
| @@ -5,6 +5,8 @@ use crate::ast::types::Range; | |||
| use crate::check_ast::Checker; | |||
| use crate::checks::{Check, CheckKind}; | |||
|
|
|||
| const FUNC_NAME_WHITELIST: &[&str] = &["get", "setdefault", "pop", "fromkeys"]; | |||
There was a problem hiding this comment.
Consider the more inclusive term “allowlist”.
If we’re going with this method name heuristic, some other candidates that come to mind are set.add, list.append, set.discard, list.count, list.index, list.insert, list.remove. There could be a lot of these, even before considering methods of custom classes. I think we should maybe start with upstream’s approach of ignoring method calls entirely?
There was a problem hiding this comment.
Yeah, at least it's an independent check code so in theory can be opted out?
There was a problem hiding this comment.
It’s not independent from the code used for non-method function calls.
$ cat test.py
def f(x):
pass
f(True)
[].append(True)
$ flake8 test.py
test.py:5:1: FBT003 do not use boolean positional args. Hint: in `f(..)`, refactor positional arg #0 to include its argument name
$ ruff --select=FBT test.py
Found 2 error(s).
test.py:5:3: FBT003 Boolean positional value in function call
test.py:6:11: FBT003 Boolean positional value in function callThere was a problem hiding this comment.
Oh sorry, I misunderstood here. What do you think, @pwoolvett?
There was a problem hiding this comment.
My gut tells me many people will have fbt003 disabled. For it to be useful, we should have an explicit allowlist. Its not a pain to maintain, and prs are easy to merge: add test case with false positive and name to allowlist. list will grow and after a couple of iterations it should be stable. Dev will have to pay the price of potential false negatives, but its still better than no fbt003.
- Custom classes should be addressed by the dev when a bt is raised on codebase.
- For cases such as when a library emits a bt, eventually an option in the toml could be used to extend the allow list. But do note the main issue here is the signature for those stdlib methods explicitly disallows named arguments with the
/token. Other cases are expected to be improved by the dev when possible by using named args. - I have nothing against splitting func (fbt003)/methods (fbt004?), keeping comments above in mind.
note: whatever is decided here will be implemented upstream eventually to keep them in sync.
If you want to debate further, maybe discussions would be a better place, but i think theyre not ensbled...
This commit enables the skipping of FBT003 based on two conditions:
{}.get(False, "default")(for boolean keys) and{}.get(key, True)(for boolean defaults). Other method signature with a name in the whitelist can still potentially generate FBT003.fixes #893