Comments (13)
assert
definitely has its place, so warning for every occurrence seems misleading. The entire point of assertions is that they can be switched off. So one actually should use them similar to how @gaul describes it: Have some costly checks that can be disabled in production.
However, -O
implies the assertion machinery is switched off. Entirely. That means two things:
- Explicitly raising
AssertionError
defeats the purpose ofassert
being optional. That means the suggestion of B011 encourages misuse of assertions. - Explicitly catching
AssertionError
leads to different-O
-behaviour at a distance. It is a better indication of misusing assertions.
A better replacement for assert False
is raise NotImplementedError
, since it usually indicates a dead code path.
However, I would rather suggest to flag except AssertionError
, perhaps flag raise AssertionError
as well. They indicate that assertions are misused for control flow, which they are not suitable for.
from flake8-bugbear.
Came here to report basically the same problem. I see this is more controversial than I expected.
Would it be acceptable to have an opinionated version of this warning (B911?) that would flag all assertions outside of tests functions and classes, assuming those are all named with test
as a case-insensitive prefix ?
from flake8-bugbear.
In my code base I have expensive assert
sanity checks on data structures which python -O
will remove in production. It has check_true
, check_not_none
, etc. helper functions for preconditions that do not use assert. Thus some of the assert
are valid for this code base but assert false
should always be replaced with raise AssertionError
.
I modeled this check after a similar one in Java error-prone which might explain my motivation more clearly:
http://errorprone.info/bugpattern/AssertFalse
from flake8-bugbear.
I like the check (because there is never a need to assert False
that I can see), but can certainly get behind modifying the advice ("replace this with by raising an appropriate exception"). What that appropriate exception is seems to be beyond the scope of a tool like this since it's highly dependent on the code and the exception hierarchy in use.
from flake8-bugbear.
I authored this check and would like to understand your concern. I have not seen a use of assert False
where the author somehow wanted to disable it via python -O
. Could you point me at an example? I also do not understand how the message misleads readers since it only flags for assert False
and not for other assert
call sites.
from flake8-bugbear.
Maybe this isn't in perfect alignment with the OP, but I'm curious why we only flag assert False
and not all assertions. I only recently learned that using assert
to detect a failure mode is an antipattern, and had to remove various assertions for this reason. If bugbear had told me sooner, I might not have put them in to begin with.
from flake8-bugbear.
I dunno. If you print a warning, then people who understand not to do it can disable the warning. If you don't print the warning, then people don't know they need it.
from flake8-bugbear.
Thanks, @maxfischer2781 ; I was using assert False
in some tests I hadn't written yet (and marking them as xfail
), and then hit B011, which led me to this page, and your reminder that raise NotImplementedError()
is a better approach in that case.
from flake8-bugbear.
My 2c: I also landed here because I was looking for how to disable this check within our test suite - which uses pytest runner - where using assert is exactly the right way to go (https://docs.pytest.org/en/7.2.x/how-to/assert.html#assert)
from flake8-bugbear.
Yeah, @rpdelaney's comment is a variant of the problem, although slightly unrelated in the sense that the advice given by this check is quite likely the best possible fix for that case: using AssertionError()
would not in my opinion very rarely if ever be an appropriate way to signal failure, there are likely better exception types for that depending on case.
But I'll try to elaborate. So, the current implementation asks to use AssertionError
instead of using assert False
. The reason it gives for that is that those assert
s get removed by -O
. But that reason is valid for (well it isn't actually a valid reason IMO, but applies similarly to) all assert
s, not just assert False
. So, to make the check consistent and not misleading, all assert
s should be flagged. But that would not be a good thing to do, because using assert
s is a valid pattern, when used properly. Hence I don't think this check is fixable.
from flake8-bugbear.
Sure, but the warning needs to be valid in some scenarios on its own. If this lead you to avoid using asserts to signal failure, that I think was a coincidence, and thus should not count in this check's favor.
Quite bluntly, as it stands here, people who understand this thing will turn it off, and those who don't are subject to make misinformed changes and develop bad practices.
from flake8-bugbear.
If this lead you to avoid using asserts to signal failure, that I think was a coincidence, and thus should not count in this check's favor.
It certainly didn't -- I didn't use bugbear until very recently. But if I had been using it, and there had been a warning for all assertions, then I would have learned about this antipattern sooner. That's what I'm saying. I think I agree that it's bad to warn only for assert False
, but it would be better to warn for all assertions than none.
from flake8-bugbear.
I also like the idea of catching places where AssertionError is caught, but it should be a separate check and disabled by default (it's valid in many cases: e.g. a unit test runner that runs other unit tests and gathers up the errors).
from flake8-bugbear.
Related Issues (20)
- B902: False positive when using the attrs library HOT 6
- Policy on bugbears in usage of popular third party libraries HOT 2
- B026 False negative on class methods HOT 1
- B017: False negative when "from" imports used HOT 2
- Improve unit test runner HOT 2
- Feature request: a rule for calling super().__init__() in custom exception's __init__() HOT 2
- Feature request: a rule for detecting calls to mutate all-caps "constants" HOT 1
- B035: False positive for comprehensions that use a walrus operator
- Couple new rule suggestions HOT 2
- Rule to detect changes to iterable object of loop HOT 5
- Error in latest version of flake8-bugbear 24.1.15 HOT 4
- B038 false positive in 24.1.15 + 24.1.16 HOT 5
- B018 doesn't trigger for useless expressions involving multiple variables HOT 3
- B909 improvements HOT 4
- B038 false positives HOT 5
- How to handle B015 within pytest.raises blocks
- B031 does not take into account if-else statements
- B909 has several false positives on black codebase HOT 7
- B023: false positive for nested function inside loop HOT 1
- B024 false negative when there's a class var
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from flake8-bugbear.