Add rule raise-missing-from #3695

cool-RR · 2020-06-16T12:18:37Z

Steps

Add yourself to CONTRIBUTORS if you are a new contributor.
Add a ChangeLog entry describing what your PR does.
If it's a new feature or an important bug fix, add a What's New entry in doc/whatsnew/<current release.rst>.
Write a good description on what the PR does.

Description

Implement #3687

Type of Changes

	Type
	🐛 Bug fix
✓	✨ New feature
	🔨 Refactoring
	📜 Docs

coveralls · 2020-06-16T15:19:12Z

Coverage increased (+0.0004%) to 90.707% when pulling fb7d5cf on cool-RR:2020-06-16-raise-missing-from into 35f838f on PyCQA:master.

cool-RR · 2020-06-16T15:59:31Z

@Pierre-Sassoulas @PCManticore

Here's my attempt to implement this feature. This is my first PyLint rule, so I'm likely to miss important things. Also I didn't write the documentation yet, because I need to know whether I'm even on the right track with this implementation. Let me know.

Pierre-Sassoulas

The codes looks fine to me, good work ! I just think there should be two or three more functional tests (using "from" explicitly to reraise). I made some suggestions.

pylint/checkers/exceptions.py

tests/functional/r/raise_missing_from.py

cool-RR · 2020-06-16T17:12:05Z

@Pierre-Sassoulas See the iterate_parents function I'm implementing. I have the feeling there's a better way to search for an enclosing except. Do you know of one?

pylint/checkers/exceptions.py

cool-RR · 2020-06-16T19:27:14Z

@Pierre-Sassoulas Any idea why Travis doesn't seem to be running?

Pierre-Sassoulas · 2020-06-16T19:46:10Z

It can be long to launch some times. If it's still not running running you can force push with a new date for the commit to trigger the pipeline.

cool-RR · 2020-06-16T19:47:04Z

I did the force push, it didn't help.

Pierre-Sassoulas · 2020-06-16T19:49:58Z

https://travis-ci.org/github/PyCQA/pylint/builds/699060729 here's the build

cool-RR · 2020-06-16T19:58:57Z

@Pierre-Sassoulas That doesn't make sense. I'm getting this error in the build:

/home/travis/build/PyCQA/pylint/pylint/checkers/exceptions.py:86:0: R1710: Either all return statements in a function should return an expression, or none of them should. (inconsistent-return-statements)

Even though I already fixed it in my code. Could it be running on an old version?

Pierre-Sassoulas · 2020-06-16T20:03:34Z

If nothing in iterate_ancestors(node) is an astroid.ExceptHandler or an astroid.scoped_nodes.LocalsDictNodeNG the function return is an implicit None. You should add return None line 96 in pylint/checkers/exceptions.py.

cool-RR · 2020-06-17T06:34:22Z

@Pierre-Sassoulas

Cool, fixed.

I thought of something. This check should only be applied in Python 3 codebases. Should I mark it in some way as a Python 3-only check?

PCManticore

Super nice PR @cool-RR ! Thanks a lot. No worries regarding marking this as a Python 3 checker, as pylint already runs only on Python 3.

PCManticore · 2020-06-17T07:02:03Z

pylint/checkers/exceptions.py

@@ -76,6 +76,26 @@ def _is_raising(body: typing.List) -> bool:
    return False


+def iterate_ancestors(node: NodeNG) -> typing.Iterator[NodeNG]:


These two functions can be replaced with pylint.checkers.utils.find_try_except_wrapper_node:

except_handler = find_try_except_wrapper_node(node) if isinstance(except_handler, astroid.ExceptHandler): # it's found

On a quick look that function will need some adjustments to make it work for this use case, but it's better than having another except handler searching routine in the code base.

It'll need 2 adjustments. One is adding an argument stop_on_function: bool = False and logic for it, that's pretty straightforward. Also consider that it's possible that the fact we haven't had this logic until now causes bugs in other checks, as code that's in a function defined in try-except doesn't have the same behavior as code that's actually in try-except. I won't dwell on that, you can open an issue on that if you'd like.

The second would be for it to find just the except clause, and not the try clause. Now, I don't know what kind of API you want for that. I can add an argument only_except: bool = False but that's not a nice API. Let me know which API you'd like.

@PCManticore

I fixed the merge conflict. Besides this issue that I'm waiting on a response on from you, is there anything else that needs to be done before this PR can be merged?

To answer your question:

The function should only look in the current scope so it should stop if it encounters a FunctionDef or a ClassDef.

We can split this function into two, one for finding TryExcept nodes, another for finding the parent ExceptHandler, or we can rely on isinstance after the function call returned to deduce the same thing.

We can split this function into two, one for finding TryExcept nodes, another for finding the parent ExceptHandler.

Okay, I pushed that now.

pylint/checkers/exceptions.py

cool-RR · 2020-06-22T07:43:14Z

Woohoo!

* Format all Python files with latest version of black (20.8b1). * Add `from` keyword when re-raising exceptions. * pylint W0707: raise-missing-from * https://pylint.readthedocs.io/en/latest/technical_reference/features.html * pylint-dev/pylint#3695

cool-RR force-pushed the 2020-06-16-raise-missing-from branch 21 times, most recently from 95c5adf to a59fd6e Compare June 16, 2020 15:16

Pierre-Sassoulas linked an issue Jun 16, 2020 that may be closed by this pull request

Feature request: Add a check for raise foo from bar #3687

Closed

Pierre-Sassoulas approved these changes Jun 16, 2020

View reviewed changes

pylint/checkers/exceptions.py Outdated Show resolved Hide resolved

pylint/checkers/exceptions.py Outdated Show resolved Hide resolved

tests/functional/r/raise_missing_from.py Show resolved Hide resolved

cool-RR force-pushed the 2020-06-16-raise-missing-from branch 2 times, most recently from 45151fa to 7998fa0 Compare June 16, 2020 17:11

cool-RR force-pushed the 2020-06-16-raise-missing-from branch from 7998fa0 to 5b561ac Compare June 16, 2020 17:14

Pierre-Sassoulas reviewed Jun 16, 2020

View reviewed changes

pylint/checkers/exceptions.py Outdated Show resolved Hide resolved

cool-RR added a commit to cool-RR/pylint that referenced this pull request Jun 16, 2020

Add rule raise-missing-from pylint-dev#3695

7af1a88

cool-RR force-pushed the 2020-06-16-raise-missing-from branch 5 times, most recently from 1ea39ff to d363377 Compare June 16, 2020 18:16

cool-RR marked this pull request as ready for review June 16, 2020 19:07

cool-RR force-pushed the 2020-06-16-raise-missing-from branch from d363377 to 4cb43b8 Compare June 16, 2020 19:08

cool-RR force-pushed the 2020-06-16-raise-missing-from branch from 4cb43b8 to f5e1485 Compare June 16, 2020 19:48

cool-RR force-pushed the 2020-06-16-raise-missing-from branch from f5e1485 to b88e989 Compare June 16, 2020 20:13

PCManticore approved these changes Jun 17, 2020

View reviewed changes

cool-RR force-pushed the 2020-06-16-raise-missing-from branch from b88e989 to 4e08206 Compare June 17, 2020 07:30

cool-RR mentioned this pull request Jun 18, 2020

Fix exception causes all over the codebase python/mypy#8998

Merged

cool-RR force-pushed the 2020-06-16-raise-missing-from branch 3 times, most recently from 1b04ea7 to 6b5bfa0 Compare June 20, 2020 10:41

Add rule raise-missing-from

fb7d5cf

cool-RR force-pushed the 2020-06-16-raise-missing-from branch from 6b5bfa0 to fb7d5cf Compare June 20, 2020 10:50

PCManticore merged commit a735857 into pylint-dev:master Jun 22, 2020

mpibpc-mroose mentioned this pull request Sep 25, 2020

Violation to force usage of the raise from exception syntax feature wemake-services/wemake-python-styleguide#1620

Closed

pseudo-rnd-thoughts mentioned this pull request Dec 10, 2022

Add linting for exception chaining with raise-from PyCQA/flake8#1765

Closed

		@@ -76,6 +76,26 @@ def _is_raising(body: typing.List) -> bool:
		return False


		def iterate_ancestors(node: NodeNG) -> typing.Iterator[NodeNG]:

Uh oh!

Add rule raise-missing-from #3695

Add rule raise-missing-from #3695

Uh oh!

Conversation

cool-RR commented Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Steps

Description

Type of Changes

Uh oh!

coveralls commented Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cool-RR commented Jun 16, 2020

Uh oh!

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cool-RR commented Jun 16, 2020

Uh oh!

Uh oh!

cool-RR commented Jun 16, 2020

Uh oh!

Pierre-Sassoulas commented Jun 16, 2020

Uh oh!

cool-RR commented Jun 16, 2020

Uh oh!

Pierre-Sassoulas commented Jun 16, 2020

Uh oh!

cool-RR commented Jun 16, 2020

Uh oh!

Pierre-Sassoulas commented Jun 16, 2020

Uh oh!

cool-RR commented Jun 17, 2020

Uh oh!

PCManticore left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PCManticore Jun 17, 2020

Choose a reason for hiding this comment

Uh oh!

cool-RR Jun 17, 2020

Choose a reason for hiding this comment

Uh oh!

cool-RR Jun 18, 2020

Choose a reason for hiding this comment

Uh oh!

cool-RR Jun 20, 2020

Choose a reason for hiding this comment

Uh oh!

PCManticore Jun 20, 2020

Choose a reason for hiding this comment

Uh oh!

cool-RR Jun 20, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cool-RR commented Jun 22, 2020

Uh oh!

Uh oh!

cool-RR commented Jun 16, 2020 •

edited

Loading

coveralls commented Jun 16, 2020 •

edited

Loading

PCManticore left a comment •

edited

Loading