[raise-missing-from] Clearer message and example in the documentation

[Pierre-Sassoulas]

Type of Changes

	Type
✓	📜 Docs

Description

Co-authored-by: @cool-RR [email protected]

Refs #5953
Closes #3707

[coveralls]

Pull Request Test Coverage Report for Build 2308633827

• 6 of 6 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.0006%) to 95.334%

---

Totals
Change from base Build 2308037244:	0.0006%
Covered Lines:	16039
Relevant Lines:	16824

---

💛 - Coveralls

[DanielNoord]

Agree with the original issue that the message description can be a lot better. Then we can probably also remove the details.rst.

[DanielNoord]

It would probably be a lot more user-friendly if we passed old_error as an argument and showed the actual error that was caught. I think we have that information when we add_message, right?

[Pierre-Sassoulas]

I started doing that but then we can have issues when there is no except ZeroDivisionError as e: but except ZeroDivisionError: instead, what should we use for old_error ?

[DanielNoord]

diff --git a/pylint/checkers/exceptions.py b/pylint/checkers/exceptions.py
index 7e182c3dc..26024f8aa 100644
--- a/pylint/checkers/exceptions.py
+++ b/pylint/checkers/exceptions.py
@@ -336,6 +336,12 @@ class ExceptionsChecker(checkers.BaseChecker):
         if containing_except_node.name is None:
             # The `except` doesn't have an `as exception:` part, meaning there's no way that
             # the `raise` is raising the same exception.
+            if isinstance(containing_except_node.type, nodes.Name):
+                name_of_old_error = containing_except_node.type.name
+            elif isinstance(containing_except_node.type, nodes.Tuple):
+                name_of_old_error = ", ".join(
+                    n.name for n in containing_except_node.type.elts
+                )
             self.add_message("raise-missing-from", node=node)
         elif isinstance(node.exc, nodes.Call) and isinstance(node.exc.func, nodes.Name):
             # We have a `raise SomeException(whatever)`.

We have access to the wrapping except handler and can determine the node name from it.

There are three other places at which the message is raised but I think we should be able to get the names there as well.

[Pierre-Sassoulas]

Say we have this:

try:
    1 / 0
except ZeroDivisionError:
    # +1: [raise-missing-from]
    raise KeyError

name_of_old_error would be ZeroDivisionError when we would want it to be e or something so the end result is this:

try:
    1 / 0
except ZeroDivisionError as e:
    raise KeyError from e

So the message should be something like "Consider explicitly re-raising using 'raise KeyError from old_error'", and if we have except ZeroDivisionError as exc: in the original code it should be Consider explicitly re-raising using 'raise KeyError from exc", right ?

[DanielNoord]

Ah. I think we should potentially rephrase the message. What about:
Consider explicitly re-raising using 'except %s as exc' and 'raise from exc'

That should work for both situations right?

[Pierre-Sassoulas]

👍 I don't know if we can do Consider explicitly re-raising using 'except %s as exc' and 'raise%s from exc' with the second args being KeyError or ValueError("my message") (depending on what the code is originally), but Consider explicitly re-raising using 'except %s as exc' and 'raise ... from exc' is probably good enough ?

[DanielNoord]

Yeah I think the ... works. Messages shouldn't be too long I think so that seems like a nice middle ground.

Is exc the most common name to do this? Docs seems to use err and exc: https://docs.python.org/3/tutorial/errors.html

[Pierre-Sassoulas]

A small refactor permitted to have a nice little message. At this point we might as well autofix files 😄 Something to consider when we have less than 10 issues opened

[DanielNoord]

Final nitpicks

[Pierre-Sassoulas]

Sorry for pushing something else after the review, I completed the coverage and fixed the suggestion for multiple exceptions caught.

[jacobtylerwalls]

I don't know if the failure was discussed above, but I found it in the primer logs:

2022-05-11T12:50:44.4952238Z ----------------------------- Captured stderr call -----------------------------
2022-05-11T12:50:44.4952358Z Traceback (most recent call last):
2022-05-11T12:50:44.4960179Z   File "/home/runner/work/pylint/pylint/pylint/utils/ast_walker.py", line 89, in walk
2022-05-11T12:50:44.4960405Z     callback(astroid)
2022-05-11T12:50:44.4960692Z   File "/home/runner/work/pylint/pylint/pylint/checkers/exceptions.py", line 272, in visit_raise
2022-05-11T12:50:44.4960846Z     self._check_raise_missing_from(node)
2022-05-11T12:50:44.4961112Z   File "/home/runner/work/pylint/pylint/pylint/checkers/exceptions.py", line 345, in _check_raise_missing_from
2022-05-11T12:50:44.4961300Z     exceptions = ", ".join(n.name for n in containing_except_node.type.elts)
2022-05-11T12:50:44.4961532Z   File "/home/runner/work/pylint/pylint/pylint/checkers/exceptions.py", line 345, in <genexpr>
2022-05-11T12:50:44.4961714Z     exceptions = ", ".join(n.name for n in containing_except_node.type.elts)
2022-05-11T12:50:44.4962039Z AttributeError: 'Attribute' object has no attribute 'name'

[Pierre-Sassoulas]

I fixed-up what was previously reviewed, the new code is in 642b18d