Use qualified name when checking for overgeneral exceptions #7497

Jackenmen · 2022-09-19T22:09:57Z

Type of Changes

	Type
✓	🐛 Bug fix
✓	🔨 Refactoring

Description

Allow specifying non-builtin exceptions in overgeneral-exception option using exception's qualified name.

Additionally, tries to deprecate specifying built-in exception names without dots. Small problem - I don't know how to tell pylint to warn about deprecation as warnings.warn(...) does not seem to be a valid way to do that.

Closes #7495

coveralls · 2022-09-19T22:17:58Z

Pull Request Test Coverage Report for Build 3455059518

14 of 15 (93.33%) changed or added relevant lines in 1 file are covered.
2 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-0.002%) to 95.38%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pylint/checkers/exceptions.py	14	15	93.33%

Files with Coverage Reduction	New Missed Lines	%
pylint/checkers/classes/class_checker.py	2	94.8%

Totals
Change from base Build 3452506349:	-0.002%
Covered Lines:	17301
Relevant Lines:	18139

💛 - Coveralls

Pierre-Sassoulas

Look good thank you ! I think this is a breaking change that we need to put in 3.0 though

Jackenmen · 2022-09-20T04:16:21Z

It keeps compatibility with the old way (names without dots are handled as they've been before and names with dots weren't supported in any way before) so I think it should be fine? The long-term goal is to remove the old way in 3.0 though.

The problem I've run into is that I don't really know how I should make pylint actually show the deprecation warning to the user (warnings.warn that's currently in here doesn't seem to work) which is why this PR is a draft.

doc/whatsnew/fragments/7495.bugfix

pylint/checkers/exceptions.py

tests/functional/b/broad_except.rc

pylint/checkers/exceptions.py

Pierre-Sassoulas · 2022-09-24T07:07:26Z

Is this still a draft @jack1142 ?

Jackenmen · 2022-09-24T16:57:09Z

Is this still a draft @jack1142 ?

I was waiting for a resolution of the warning discussion before committing any changes. Should be ready for review now.

Pierre-Sassoulas

Looks pretty refined already, good job !

pylint/checkers/exceptions.py

Pierre-Sassoulas · 2022-09-24T17:45:54Z

pylint/checkers/exceptions.py

+                                "broad-except",
+                                args=exception.name,
+                                node=handler.type,
+                                confidence=INFERENCE,


pylint/checkers/exceptions.py

Pierre-Sassoulas

LGTM 👏

DanielNoord · 2022-11-09T19:42:48Z

Shit.. This needs a rebase due to the conflicts. I'm sorry about that @jack1142.

I've been very busy. I promise that if you rebase this PR and tag me I'll make sure to merge it immediately after and not let this slip 😅

Jackenmen · 2022-11-09T19:46:12Z

Damn, conflict with 4 files 😄 I will do it tomorrow as I'm a bit busy tonight, thanks for letting me know!

DanielNoord · 2022-11-09T19:48:41Z

Yeah this is our bad probably. I have recently started a new job and have had less time for OS. This has messed a bit with out review/merge flow I think.

Like I said, make sure to ping my directly as I'll be more likely to see and follow up on it 😄

Pierre-Sassoulas · 2022-11-09T20:00:58Z

Probably due to #7709, you're going to have to rename old broad-except and consider the new check too, sorry for letting this go stale.

Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>

Jackenmen · 2022-11-10T12:45:02Z

broad-exception-caught doesn't get triggered if the except block raises so these test additions don't seem to make sense

We could add few more try except blocks which ensure that message isn't reported I suppose.

Pierre-Sassoulas

You're right, sorry for the noise !

pylint/checkers/exceptions.py

jacobtylerwalls

I'm concerned about the changes shown in the primer. Most of the patterns there look benign, where a broad exception is caught, and then some logging/cleanup is done, and then the original error is re-raised. I didn't think we had consensus on changing that (or at least, I don't see any discussion in the issue.) I think the status quo should be preserved where directly raising MyBroadException emits a message, but catching and reraising it doesn't.

pylint/checkers/exceptions.py

Jackenmen · 2022-11-13T03:07:34Z

I'm concerned about the changes shown in the primer. Most of the patterns there look benign, where a broad exception is caught, and then some logging/cleanup is done, and then the original error is re-raised. I didn't think we had consensus on changing that (or at least, I don't see any discussion in the issue.)

Yeah, there was definitely no consensus on this change. The primer workflow didn't manage to run successfully on this PR 3 days ago when I added inference to broad-exception-raised so I wasn't really aware of these cases.

I think the status quo should be preserved where directly raising MyBroadException emits a message, but catching and reraising it doesn't.

I agree, I've done some changes that should hopefully address this.

jacobtylerwalls

Thanks for the quick updates! (And sorry the primer was down.)

Jackenmen · 2022-11-13T04:10:21Z

All good, I'm happy you/primer caught it before this PR was merged :)

DanielNoord

Thanks a lot @jack1142, especially for revisiting and fixing the conflicts after we forgot to merge this.

Much appreciated!

pylint/checkers/exceptions.py

github-actions · 2022-11-13T12:06:37Z

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit b480242

Pierre-Sassoulas · 2022-11-13T12:41:55Z

@DanielNoord please cleanup the commit message when squashing, it does not have to be super fancy and a perefect synthesis but this one was just a mess 😄

Use qualified name when checking for overgeneral exceptions (https://github.com/PyCQA/pylint/pull/7497)

* Update tests

* Use qualified name when checking for overgeneral exceptions

* WIP: Add deprecation warning

* Add changelog fragment

* Use qualified name in test case

* spell check fix

* Update changelog fragment with suggested fixes

Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>

* Move OVERGENERAL_EXCEPTIONS directly to the default value in dict

* Mark as TODO for pylint 3.0

* Properly warn on each occurrence of name without dots

* Update the warning per the review

* Rephrase the warning to mention pylint 3.0

* Remove unnecessary nesting of the if condition

* Quote the exception name in deprecation warning

* Use config value for overgeneral exceptions in broad-exception-raised

* Infer qualified name of the exception in broad-exception-raised

* e.g. -> maybe?

Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>

* Suppress missing class docstrings

* Add few more tests for broad-exception-raised

Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>

* Fix unexpected missing-raise-from

* Revert "Fix unexpected missing-raise-from"

This reverts commit https://github.com/PyCQA/pylint/commit/d796e72035b7f7578b9e6bb1e45a30935e80b009.

* Revert "Add few more tests for broad-exception-raised"

This reverts commit https://github.com/PyCQA/pylint/commit/e5a193ee136f8566d43450fbb9fbf28cc717d307.

* Change confidence of broad-exception-raised from HIGH to INFERENCE

* Only trigger broad-exception-raised for raise with new exc instance

* Update overgeneral-exceptions definition in example pylintrc file

* Update pylint/checkers/exceptions.py

Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>
Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>

DanielNoord · 2022-11-13T12:49:23Z

Shit, I thought I did. Sorry about that!

cdce8p · 2022-11-16T09:35:00Z

pylint/checkers/exceptions.py

+                warnings.warn_explicit(
+                    "Specifying exception names in the overgeneral-exceptions option"
+                    " without module name is deprecated and support for it"
+                    " will be removed in pylint 3.0."
+                    f" Use fully qualified name (maybe 'builtins.{exc_name}' ?) instead.",
+                    category=UserWarning,
+                    filename="pylint: Command line or configuration file",
+                    lineno=1,
+                    module="pylint",
+                )


With --jobs 2, the open() call is executed for each opened file. E.g. for HomeAssistant I got over 16.000 warnings just for 3 old config values. That should be fixed before we release 2.16.
https://github.com/cdce8p/ha-core/actions/runs/3470026809/jobs/5797737991#step:8:25

pylint: Command line or configuration file:1: UserWarning: Specifying exception names in the overgeneral-exceptions option without module name is deprecated and support for it will be removed in pylint 3.0. Use fully qualified name (maybe 'builtins.BaseException' ?) instead. pylint: Command line or configuration file:1: UserWarning: Specifying exception names in the overgeneral-exceptions option without module name is deprecated and support for it will be removed in pylint 3.0. Use fully qualified name (maybe 'builtins.Exception' ?) instead. pylint: Command line or configuration file:1: UserWarning: Specifying exception names in the overgeneral-exceptions option without module name is deprecated and support for it will be removed in pylint 3.0. Use fully qualified name (maybe 'builtins.HomeAssistantError' ?) instead. pylint: Command line or configuration file:1: UserWarning: Specifying exception names in the overgeneral-exceptions option without module name is deprecated and support for it will be removed in pylint 3.0. Use fully qualified name (maybe 'builtins.BaseException' ?) instead. pylint: Command line or configuration file:1: UserWarning: Specifying exception names in the overgeneral-exceptions option without module name is deprecated and support for it will be removed in pylint 3.0. Use fully qualified name (maybe 'builtins.Exception' ?) instead. pylint: Command line or configuration file:1: UserWarning: Specifying exception names in the overgeneral-exceptions option without module name is deprecated and support for it will be removed in pylint 3.0. Use fully qualified name (maybe 'builtins.HomeAssistantError' ?) instead. pylint: Command line or configuration file:1: UserWarning: Specifying exception names in the overgeneral-exceptions option without module name is deprecated and support for it will be removed in pylint 3.0. Use fully qualified name (maybe 'builtins.BaseException' ?) instead. pylint: Command line or configuration file:1: UserWarning: Specifying exception names in the overgeneral-exceptions option without module name is deprecated and support for it will be removed in pylint 3.0. Use fully qualified name (maybe 'builtins.Exception' ?) instead. pylint: Command line or configuration file:1: UserWarning: Specifying exception names in the overgeneral-exceptions option without module name is deprecated and support for it will be removed in pylint 3.0. Use fully qualified name (maybe 'builtins.HomeAssistantError' ?) instead. pylint: Command line or configuration file:1: UserWarning: Specifying exception names in the overgeneral-exceptions option without module name is deprecated and support for it will be removed in pylint 3.0. Use fully qualified name (maybe 'builtins.BaseException' ?) instead. ...

This comment has been minimized.

Sign in to view

Pierre-Sassoulas added the Enhancement ✨ Improvement to a component label Sep 20, 2022

Pierre-Sassoulas added this to the 2.16.0 milestone Sep 20, 2022

Pierre-Sassoulas reviewed Sep 20, 2022

View reviewed changes

Pierre-Sassoulas added Needs review 🔍 Needs to be reviewed by one or multiple more persons Needs specification 🔐 Accepted as a potential improvement, and needs to specify edge cases, message names, etc. labels Sep 20, 2022

DanielNoord requested changes Sep 20, 2022

View reviewed changes

DanielNoord removed the Needs review 🔍 Needs to be reviewed by one or multiple more persons label Sep 20, 2022

This comment has been minimized.

Sign in to view

Jackenmen marked this pull request as ready for review September 24, 2022 16:57

Pierre-Sassoulas removed the Needs specification 🔐 Accepted as a potential improvement, and needs to specify edge cases, message names, etc. label Sep 24, 2022

Pierre-Sassoulas reviewed Sep 24, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

Pierre-Sassoulas previously approved these changes Sep 24, 2022

View reviewed changes

Jackenmen and others added 8 commits November 10, 2022 09:10

Update tests

49822c2

Use qualified name when checking for overgeneral exceptions

fd2e7f0

WIP: Add deprecation warning

6457421

Add changelog fragment

2ab4c8a

Use qualified name in test case

773a5b6

spell check fix

0a5ec78

Update changelog fragment with suggested fixes

ed0529a

Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>

Move OVERGENERAL_EXCEPTIONS directly to the default value in dict

2bc4e33

Pierre-Sassoulas previously approved these changes Nov 10, 2022

View reviewed changes

Pierre-Sassoulas enabled auto-merge (squash) November 10, 2022 12:57

DanielNoord requested changes Nov 12, 2022

View reviewed changes

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

auto-merge was automatically disabled November 12, 2022 20:52
Head branch was pushed to by a user without write access

Jackenmen dismissed Pierre-Sassoulas’s stale review via 064d8e6 November 12, 2022 20:52

Jackenmen requested a review from DanielNoord November 12, 2022 20:52

Change confidence of broad-exception-raised from HIGH to INFERENCE

fe61ab4

This comment has been minimized.

Sign in to view

jacobtylerwalls self-requested a review November 12, 2022 22:16

jacobtylerwalls requested changes Nov 12, 2022

View reviewed changes

pylint/checkers/exceptions.py Show resolved Hide resolved

Jackenmen added 2 commits November 13, 2022 04:04

Only trigger broad-exception-raised for raise with new exc instance

12bef81

Update overgeneral-exceptions definition in example pylintrc file

0597dc1

jacobtylerwalls previously approved these changes Nov 13, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

DanielNoord previously approved these changes Nov 13, 2022

View reviewed changes

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

Update pylint/checkers/exceptions.py

b480242

DanielNoord dismissed stale reviews from jacobtylerwalls and themself via b480242 November 13, 2022 11:26

DanielNoord enabled auto-merge (squash) November 13, 2022 11:27

Pierre-Sassoulas approved these changes Nov 13, 2022

View reviewed changes

DanielNoord merged commit ac65bdc into pylint-dev:main Nov 13, 2022

Jackenmen deleted the issue_7495 branch November 13, 2022 12:26

cdce8p reviewed Nov 16, 2022

View reviewed changes

Pierre-Sassoulas mentioned this pull request Nov 16, 2022

With --jobs 2, the open() call is executed for each opened file #7774

Closed

Uh oh!

Use qualified name when checking for overgeneral exceptions #7497

Use qualified name when checking for overgeneral exceptions #7497

Uh oh!

Conversation

Jackenmen commented Sep 19, 2022

Type of Changes

Description

Uh oh!

coveralls commented Sep 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 3455059518

💛 - Coveralls

Uh oh!

This comment has been minimized.

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Uh oh!

Jackenmen commented Sep 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

Pierre-Sassoulas commented Sep 24, 2022

Uh oh!

Jackenmen commented Sep 24, 2022

Uh oh!

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Pierre-Sassoulas Sep 24, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment has been minimized.

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Uh oh!

DanielNoord commented Nov 9, 2022

Uh oh!

Jackenmen commented Nov 9, 2022

Uh oh!

DanielNoord commented Nov 9, 2022

Uh oh!

Pierre-Sassoulas commented Nov 9, 2022

Uh oh!

Jackenmen commented Nov 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment has been minimized.

jacobtylerwalls left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Jackenmen commented Nov 13, 2022

Uh oh!

jacobtylerwalls left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

Jackenmen commented Nov 13, 2022

Uh oh!

DanielNoord left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Nov 13, 2022

coveralls commented Sep 19, 2022 •

edited

Loading

Jackenmen commented Sep 20, 2022 •

edited

Loading

Jackenmen commented Nov 10, 2022 •

edited

Loading

cdce8p Nov 16, 2022 •

edited

Loading