Fix exception causes in config/init.py #7351

cool-RR · 2020-06-11T17:28:10Z

I recently went over Matplotlib, Pandas and NumPy, fixing a small mistake in the way that Python 3's exception chaining is used. If you're interested, I can do it here too. I've done it on just one file right now.

The mistake is this: In some parts of the code, an exception is being caught and replaced with a more user-friendly error. In these cases the syntax raise new_error from old_error needs to be used.

Python 3's exception chaining means it shows not only the traceback of the current exception, but that of the original exception (and possibly more.) This is regardless of raise from. The usage of raise from tells Python to put a more accurate message between the tracebacks. Instead of this:

During handling of the above exception, another exception occurred:

You'll get this:

The above exception was the direct cause of the following exception:

The first is inaccurate, because it signifies a bug in the exception-handling code itself, which is a separate situation than wrapping an exception.

Let me know what you think!

nicoddemus · 2020-06-11T17:37:55Z

I'm 👍 on the idea, thanks for the offer!

Are there still more places in the codebase where this adjustment is needed, or this PR covers them all?

cool-RR · 2020-06-11T17:39:40Z

Awesome!

No, there are a few more:

src/_pytest/_code/source.py: 218
src/_pytest/config/__init__.py: 642
src/_pytest/config/__init__.py: 1134
src/_pytest/config/__init__.py: 1201
src/_pytest/config/__init__.py: 1233
src/_pytest/config/argparsing.py: 270
src/_pytest/config/findpaths.py: 29
src/_pytest/debugging.py: 30
src/_pytest/debugging.py: 129
src/_pytest/fixtures.py: 942
src/_pytest/logging.py: 492
src/_pytest/monkeypatch.py: 64
src/_pytest/monkeypatch.py: 73
src/_pytest/python.py: 562
src/_pytest/python.py: 564
src/_pytest/python.py: 583
src/_pytest/python.py: 592
src/_pytest/python.py: 847
src/_pytest/python.py: 1085
src/_pytest/runner.py: 160
src/_pytest/runner.py: 428
src/_pytest/warnings.py: 52
testing/test_config.py: 1647
testing/test_runner.py: 538

Some of these will be false-positives, I'll go over all of them and give you a separate PR after we're done with this PR.

nicoddemus · 2020-06-11T17:41:18Z

Thanks!

Wouldn't be better to just change them in this PR instead though?

cool-RR · 2020-06-11T17:45:21Z

I could do that, but before I spend time going over these cases, I want to be sure there wouldn't be any roadblock. Getting this PR merged would be a good indication on whether there'll be a roadblock.

nicoddemus · 2020-06-11T17:47:02Z

Well the tests passing is a good indicator that things will probably go smooth. 😉

But let's wait for other maintainers to chime in before you spend more time on this. 👍

bluetech

Definitely +1 from me on proper exception chaining.

I've pointed several places where I think from None is more appropriate.

bluetech · 2020-06-11T19:26:07Z

src/_pytest/config/__init__.py

+        except KeyError as e:
+            raise ValueError("unknown configuration value: {!r}".format(name)) from e


Here I think from None would be better.

Suggested change

except KeyError as e:

raise ValueError("unknown configuration value: {!r}".format(name)) from e

except KeyError:

raise ValueError("unknown configuration value: {!r}".format(name)) from None

bluetech · 2020-06-11T19:26:25Z

src/_pytest/config/__init__.py

@@ -1223,14 +1223,14 @@ def getoption(self, name: str, default=notset, skip: bool = False):
            if val is None and skip:
                raise AttributeError(name)
            return val
-        except AttributeError:
+        except AttributeError as e:


Suggested change

except AttributeError as e:

except AttributeError:

bluetech · 2020-06-11T19:26:35Z

src/_pytest/config/__init__.py

            if default is not notset:
                return default
            if skip:
                import pytest

                pytest.skip("no {!r} option found".format(name))
-            raise ValueError("no option named {!r}".format(name))
+            raise ValueError("no option named {!r}".format(name)) from e


Suggested change

raise ValueError("no option named {!r}".format(name)) from e

raise ValueError("no option named {!r}".format(name)) from None

bluetech · 2020-06-11T19:27:38Z

src/_pytest/config/__init__.py

@@ -641,7 +641,7 @@ def import_plugin(self, modname: str, consider_entry_points: bool = False) -> No
        except ImportError as e:
            raise ImportError(
                'Error importing plugin "{}": {}'.format(modname, str(e.args[0]))
-            ).with_traceback(e.__traceback__)
+            ).with_traceback(e.__traceback__) from e


The with_traceback makes me think maybe the intention here is from None.

cool-RR · 2020-06-11T19:58:46Z

My position is that I prefer from old_exception everywhere. I'm okay with skipping a few cases that we don't have agreement on, but I won't add from None, because I don't want to be the reason that someone debugging a problem doesn't have access to potentially helpful information. I'm okay with anyone else adding from None on a separate PR.

If you agree, I'll just remove my changes for all the cases where you've done from None and leave just the ones we agree about.

bluetech · 2020-06-11T20:05:25Z

@cool-RR Of course I agree :) I'd be interested to understand your position though.

For example in this case

try:
    description, type, default = self._parser._inidict[name]
except KeyError as e:
    raise ValueError("unknown configuration value: {!r}".format(name)) from e

The KeyError I think would be plain noise, exposing an internal implementation detail. It could have been written just the same as

if name in self._parser._inidict:
    description, type, default = self._parser._inidict[name]
else:
    raise ValueError("unknown configuration value: {!r}".format(name))

cool-RR · 2020-06-11T20:10:20Z

@bluetech

I'll happily explain my position :)

I personally hate from None, and I think that when people are debugging they have a very different mindset than when they are writing code. When you're debugging, and you get a very limited amount of information that may have been copy-pasted in an email by a non-technical person, you're very desperate for the information.

In this case, I would have wanted to see the self._parser._inidict[name] traceback. I would maybe get a hint on what happened. If it's a dead end for exploration, I at least want to know its content before I rule it out as a dead end.

Another issue is that some error-reporting systems show not only the traceback, but all the local variables on each of these levels. These supposedly irrelevant levels could have variables that make it clearer what this exception is about.

bluetech · 2020-06-12T15:21:24Z

Thanks for explaining your position! So in some cases, like debugging crashes or unexpected exceptions, I fully agree. But in the cases in the PR, the intention is to report some error to the user, and here I subscribe to a different approach, which is to make the errors as concise and to-the-point as possible.

But, I shall not force you to go against your principles 😄 Since a direct cause is already an improvement over an indirect cause (which to me in Python 3 means a programmer error), it's fine by me.

cool-RR · 2020-06-12T15:53:33Z

Cool :)

I changed the cases you mentioned, except the one on line 644. I think that the .with_traceback(e.__traceback__), which can actually be removed, means that the traceback of the original exception is relevant for understanding the problem.

bluetech · 2020-06-14T15:04:07Z

Thanks @cool-RR!

cool-RR · 2020-06-24T08:34:02Z

I just made a blog post about this change here: https://blog.ram.rachum.com/post/621791438475296768/improving-python-exception-chaining-with

nicoddemus approved these changes Jun 11, 2020

View reviewed changes

bluetech reviewed Jun 11, 2020

View reviewed changes

Fix exception causes in config/__init__.py

caa984c

cool-RR force-pushed the 2020-06-11-raise-from branch from 129dbed to caa984c Compare June 12, 2020 15:50

bhrutledge mentioned this pull request Jun 14, 2020

Fix exception causes in package.py pypa/twine#662

Closed

bluetech merged commit 4f4c263 into pytest-dev:master Jun 14, 2020

cool-RR mentioned this pull request Jun 19, 2020

Fix exception causes all over the codebase #7387

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix exception causes in config/init.py #7351

Fix exception causes in config/init.py #7351

cool-RR commented Jun 11, 2020

nicoddemus commented Jun 11, 2020

cool-RR commented Jun 11, 2020

nicoddemus commented Jun 11, 2020

cool-RR commented Jun 11, 2020

nicoddemus commented Jun 11, 2020

bluetech left a comment

bluetech Jun 11, 2020

bluetech Jun 11, 2020

bluetech Jun 11, 2020

bluetech Jun 11, 2020

cool-RR commented Jun 11, 2020

bluetech commented Jun 11, 2020

cool-RR commented Jun 11, 2020

bluetech commented Jun 12, 2020

cool-RR commented Jun 12, 2020

bluetech commented Jun 14, 2020

cool-RR commented Jun 24, 2020

		except KeyError as e:
		raise ValueError("unknown configuration value: {!r}".format(name)) from e

	raise ValueError("no option named {!r}".format(name)) from e
	raise ValueError("no option named {!r}".format(name)) from None

Fix exception causes in config/__init__.py #7351

Fix exception causes in config/__init__.py #7351

Conversation

cool-RR commented Jun 11, 2020

nicoddemus commented Jun 11, 2020

cool-RR commented Jun 11, 2020

nicoddemus commented Jun 11, 2020

cool-RR commented Jun 11, 2020

nicoddemus commented Jun 11, 2020

bluetech left a comment

Choose a reason for hiding this comment

bluetech Jun 11, 2020

Choose a reason for hiding this comment

bluetech Jun 11, 2020

Choose a reason for hiding this comment

bluetech Jun 11, 2020

Choose a reason for hiding this comment

bluetech Jun 11, 2020

Choose a reason for hiding this comment

cool-RR commented Jun 11, 2020

bluetech commented Jun 11, 2020

cool-RR commented Jun 11, 2020

bluetech commented Jun 12, 2020

cool-RR commented Jun 12, 2020

bluetech commented Jun 14, 2020

cool-RR commented Jun 24, 2020

Fix exception causes in config/init.py #7351

Fix exception causes in config/init.py #7351