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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add B904 - Use raise ... from err in except clauses #181

Merged
merged 5 commits into from Sep 9, 2021
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion .flake8
Expand Up @@ -2,7 +2,7 @@
# Keep in sync with setup.cfg which is used for source packages.

[flake8]
ignore = E203, E302, E501, E999
ignore = E203, E302, E501, E999, W503
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why add this? Just your personal preference? I usually follow it and move the operator.

Copy link
Member Author

@Zac-HD Zac-HD Aug 23, 2021

Choose a reason for hiding this comment

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

Following black's recommendation - W503 is actually disabled by default, but enabled for bugbear by the W in select = B,C,E,F,W,B9.

I updated the config because I also added the first case of this pattern, in check_for_b018() here, and black insists on that placement of the operator.

Copy link

Choose a reason for hiding this comment

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

Maybe use extend-ignore instead of ignore here to not override the defaults?

W503 for instance is off by default unless defaults are overridden by ignore configuration.

max-line-length = 88
max-complexity = 12
select = B,C,E,F,W,B9
20 changes: 15 additions & 5 deletions README.rst
Expand Up @@ -123,9 +123,9 @@ waste CPU instructions. Either prepend ``assert`` or remove it.
**B016**: Cannot raise a literal. Did you intend to return it or raise
an Exception?

**B017**: ``self.assertRaises(Exception):`` should be considered evil. It can lead
to your test passing even if the code being tested is never executed due to a typo.
Either assert for a more specific exception (builtin or custom), use
**B017**: ``self.assertRaises(Exception):`` should be considered evil. It can lead
to your test passing even if the code being tested is never executed due to a typo.
Either assert for a more specific exception (builtin or custom), use
Zac-HD marked this conversation as resolved.
Show resolved Hide resolved
``assertRaisesRegex``, or use the context manager form of assertRaises
(``with self.assertRaises(Exception) as ex:``) with an assertion against the
data available in ``ex``.
Expand Down Expand Up @@ -157,6 +157,11 @@ nothing else. If the attributes should be mutable, define the attributes
in ``__slots__`` to save per-instance memory and to prevent accidentally
creating additional attributes on instances.

**B904**: Within an ``except`` clause, raise exceptions with ``raise ... from err``
or ``raise ... from None`` to distinguish them from errors in exception handling.
See [the exception chaining tutorial](https://docs.python.org/3/tutorial/errors.html#exception-chaining)
for details.

**B950**: Line too long. This is a pragmatic equivalent of
``pycodestyle``'s E501: it considers "max-line-length" but only triggers
when the value has been exceeded by **more than 10%**. You will no
Expand Down Expand Up @@ -217,10 +222,15 @@ MIT
Change Log
----------

Future
~~~~~~

* Add B904: check for ``raise`` without ``from`` in an ``except`` clause

21.4.3
~~~~~~

* Verify the element in item_context.args is of type ast.Name for b017
* Verify the element in item_context.args is of type ast.Name for b017

21.4.2
~~~~~~
Expand All @@ -231,7 +241,7 @@ Change Log
~~~~~~

* Add B017: check for gotta-catch-em-all assertRaises(Exception)

21.3.2
~~~~~~

Expand Down
26 changes: 25 additions & 1 deletion bugbear.py
Expand Up @@ -292,6 +292,7 @@ def visit_Compare(self, node):

def visit_Raise(self, node):
self.check_for_b016(node)
self.check_for_b904(node)
self.generic_visit(node)

def visit_With(self, node):
Expand Down Expand Up @@ -426,6 +427,21 @@ def check_for_b017(self, node):
):
self.errors.append(B017(node.lineno, node.col_offset))

def check_for_b904(self, node):
"""Checks `raise` without `from` inside an `except` clause.

In these cases, you should use explicit exception chaining from the
earlier error, or suppress it with `raise ... from None`. See
https://docs.python.org/3/tutorial/errors.html#exception-chaining
"""
if (
node.cause is None
and node.exc is not None
and not (isinstance(node.exc, ast.Name) and node.exc.id.islower())
and any(isinstance(n, ast.ExceptHandler) for n in self.node_stack)
):
self.errors.append(B904(node.lineno, node.col_offset))

def walk_function_body(self, node):
def _loop(parent, node):
if isinstance(node, (ast.AsyncFunctionDef, ast.FunctionDef)):
Expand Down Expand Up @@ -775,6 +791,14 @@ def visit(self, node):
)
)

B904 = Error(
message=(
"B904 Within an `except` clause, raise exceptions with `raise ... from err` "
"or `raise ... from None` to distinguish them from errors in exception handling. "
"See https://docs.python.org/3/tutorial/errors.html#exception-chaining for details."
)
)

B950 = Error(message="B950 line too long ({} > {} characters)")

disabled_by_default = ["B901", "B902", "B903", "B950"]
disabled_by_default = ["B901", "B902", "B903", "B904", "B950"]
21 changes: 21 additions & 0 deletions tests/b904.py
@@ -0,0 +1,21 @@
"""
Should emit:
B904 - on lines 10, 11 and 16
"""

try:
raise ValueError
except ValueError:
if "abc":
raise TypeError
raise UserWarning
except AssertionError:
raise # Bare `raise` should not be an error
except Exception as err:
assert err
raise Exception("No cause here...")
except BaseException as base_err:
# Might use this instead of bare raise with the `.with_traceback()` method
raise base_err
finally:
raise Exception("Nothing to chain from, so no warning here")
12 changes: 12 additions & 0 deletions tests/test_bugbear.py
Expand Up @@ -28,6 +28,7 @@
B015,
B016,
B017,
B904,
B901,
B902,
B903,
Expand Down Expand Up @@ -260,6 +261,17 @@ def test_b903(self):
errors = list(bbc.run())
self.assertEqual(errors, self.errors(B903(32, 0), B903(38, 0)))

def test_b904(self):
filename = Path(__file__).absolute().parent / "b904.py"
bbc = BugBearChecker(filename=str(filename))
errors = list(bbc.run())
expected = [
B904(10, 8),
B904(11, 4),
B904(16, 4),
]
self.assertEqual(errors, self.errors(*expected))

def test_b950(self):
filename = Path(__file__).absolute().parent / "b950.py"
bbc = BugBearChecker(filename=str(filename))
Expand Down