If unittest’s subTest was written after this PEP was accepted, it should use exception notes to attach the value to the exception (and ExceptionGroup to group individual failures).
You can definitely have nested loops with subTest.
A GUI traceback formatter.
The initial single-string __note__ design can accommodate all that if we maintain the convention that multiple notes should be separated by a single \n and individual notes should not contain \n characters, lest they be considered to be multiple notes by code picking apart notes (using str.split('\n')). That works well enough for print(x), it should work well enough here.
AFAICS, a single \n doesn’t work well with Hypothesis, which may need to show values of several variables in its “falsifying example” note.
AFAIK Hypothesis never takes notes apart so it doesn’t care whether what it adds is considered to be a single note or multiple notes. It just cares that there is a nicely formatted message for the human user to read.
I guess we’re back to the use case of a GUI traceback formatter that wants to put some extra space between notes, or a box around each note, or something. It must still honor the \n characters and render them as line breaks, not spaces, else it would make a jumble of Hypothesis’s efforts.
I find that a pretty weak use case for doing something more complicated than a single string – displaying all the notes as a text box using linefeeds to render \n feels fine to me. (Bonus points for not destroying leading whitespace. 
While I personally prefer the current .add_note(note)-and-__notes__ approach which supports later separate handling of each note, I’m also fine with the simple string-or-None __note__ that we originally proposed and shipped if the SC prefers it.
What’s proposed is deservedly up to you, as the PEP author, who spends most time thinking about it and the implementation.
The main reason I posted here again is that I do I find the Rejected Ideas section “ Allow any object, and convert to string for display” and “ Just make __notes__ mutable” that depends on it unconvincing, as they’re written now. It’s possible to avoid converting to string by using a fixed “non-string note” message, and getting error at the location where a bad type was assigned is not a blocker issue.
FWIW, since I know a project that takes pride in pretty-printing tracebacks – Rich – I asked for their opinion. Maybe they have useful ideas.
I agree with Guido that the use cases for anything other than adding a note and letting it appear in the traceback are very rare. But I would keep add_note and the immutable tuple (because sooner or later someone will need the individual notes for something, and this supports that). But we should revert the over-specification of how it is implemented (the last change we made).
Okay, that makes sense to me. Hopefully it will make sense to the SC.
One simplification we have knocked about on the SC while discussing this PEP is:
class Exception:
def add_note(self, note: str) -> None:
if not hasattr(self, "__note__"):
self.__note__ = []
self.__note__.append(note)
This keeps the memory down as no notes means no __note__, while also letting multiple notes exist. And there’s no need for a descriptor to handle del exc.__note__.
I’m not sure if that’s too simple, but it does seem to cover the use-cases and handles the memory concern.
Works for me. You could potentially get an error if someone adds a __notes__ that’s not a list (or technically, doesn’t support .append()), but that’s fine with me as long as we clearly define the interface we expect for __notes__.
We’d need to define also what the traceback code should do when this happens.
Personally, if you assume the attribute is either not defined or a list of strings, then you with that and if there’s an exception while handling it either swallow it or print a warning after the traceback or something. So "\n".join(exc.__notes__) would be acceptable and if it raises an exception, either swallow it, print the traceback saying, “there was an issue with the notes: {exc.notes!r}”, or something else like warnings.warn() if you want to make the warning “formal”.
Ok, to try this I’ve updated PEP-678: implementation of exception notes as per PEP discussions by iritkatriel · Pull Request #31317 · python/cpython · GitHub with a version that just ignores invalid notes (.__notes__ is not a sequence or an item in it is not a str)
I like where this is going with that PR and the discussion on tightening up that implementation. (I haven’t read its code)
In general from the SC point of view the PEP 678 text today seems overly complicated - thus comments from several of us hoping for a simpler state. From the whole always present additional attribute thing that is a tuple, to requiring strs. That was too much. Where this is now headed with the existence of the attribute itself indicating if there are any notes and it merely being a list of things that get printed (an implied str() call on them) with the creation of the attribute handled by the add_note method feels a lot less complicated and more natural with no instance overhead when notes go unused.
We recommend updating the PEP text to match the simpler version you’re aiming that PR at.
I have one concern about this PEP is that there is no control on whether to add note before or after the original message.
For context, I myself have developped a util to reraise Exception without the During handling of the above exception ... boilerplate: etils/reraise_utils.py at main · google/etils · GitHub
It seems this is exactly the problem this pep try to solve.
This util has been used across many Google projects (TFDS, SunDs, Visu3d, ML Pathways, Jax3d,…).
with epy.maybe_reraise(prefix=f'Error in {fn.__name__}: '):
fn()
My experience is that most time, I want the additional info to be added before the original message, but occasionally I want the additional info to be added after.
For example:
ValueError: Error in `my_fn`: <original message>
ValueError: Error for feature `some key`: <original message>
ValueError: Failed to encode example {example}: <original message>
This is especially true when reraise are nested (e.g. error raised in feature0/nested_feature/feat/):
ValueError:
Error in feature0/
Error in nested_feature/
Error in feat/
<original message>
Having the feat/ displayed in reverse order would be confusing I think.
KeyError: <original message>. Did you mean `existing_key`
ValueError: <original message>. Valid file formats: `.png`, `.jpg`
ImportError: <original message>. Optional deps need to be installed with `pip install my_project[dev]`
If you want to see examples of usage: you can look in TensorFlow Dataset: GitHub Code Search (Preview)
Currently it seems this PEP does not mention this point while I feel this is a quite important one.
Thank you for this @Conchylicultor. I think this would be great to mention this in the PEP. It shows (1) that there is a need for this feature, and (2) the limitations of trying to implement it without language support.
Your implementation is based on assumptions about exceptions which do not hold in general, such as the signature of __init__ (your wrapper won’t work with PEP 654 Exception Groups, for instance).
The problem you raised regarding the order of notes in
ValueError:
Error in feature0/
Error in nested_feature/
Error in feat/
<original message>
doesn’t really exist with PEP 678 - you can reorder the notes in the __notes__ list as you want and they will appear in this order. They will still appear after the exception’s str (is this a problem?), but they won’t stack up in a confusing way, like what you get from multiple wrappers.
Yesterday I came across another interesting bit of history that I think should be mentioned in the PEP:
https://bugs.python.org/issue19585
and the linked discussion at
https://mail.python.org/pipermail/python-dev/2013-November/130155.html
The latter looks very much like what PEP-678 is suggesting, but it too makes an unwarranted assumption about exceptions (namely that they don’t override BaseException.__str__).
Thank you for the answer. Yes, I think there’s a real need for this feature.
Your implementation is based on assumptions about exceptions which do not hold in general, such as the signature of
__init__
epy.reraise does not make any assumptions about the exception and works with arbitrary __init__ signature. As shown in the tests: etils/etils/epy/reraise_utils_test.py at main · google/etils · GitHub
I’m using hacks (like binding the original exception through closure to bypass BaseException.__new__) to make this work.
I haven’t tested with exception group though.
They will still appear after the exception’s str (is this a problem?)
At least for me, I want to keep control how the final exception is rendered. For example:
with reraise(suffix=f'Could not load database. This could indicate an error in the specs.'):
with reraise(prefix=f'Error in {fn.__name__}:'):
with reraise(prefix=f'Error in: {fn.__name__}:'):
with reraise(suffix=f'Did you mean `{key}` ?'):
raise KeyError(f'Invalid key `{k}`')
Would like to be rendered as:
Error in `top_function`:
Error in `nested_function`:
Invalid key: `wrong_key`
Did you mean `correct_key` ?
Could not load database. This could indicate an error in the specs
The test shows that it works for the signature def __init__(self, *args, **kwargs), not for any (arbitrary) signature.
with maybe_reraise("Blah"):
raise ExceptionGroup("msg", [TypeError(1)])
outputs this:
+ Exception Group Traceback (most recent call last):
| File "/Users/iritkatriel/src/cpython/xx.py", line 114, in maybe_reraise
| yield
| ^^^^^
| File "/Users/iritkatriel/src/cpython/xx.py", line 119, in <module>
| raise ExceptionGroup("msg", [TypeError(1)])
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| ExceptionGroup: msg
+-+---------------- 1 ----------------
| TypeError: 1
+------------------------------------
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/Users/iritkatriel/src/cpython/xx.py", line 118, in <module>
with maybe_reraise("Blah"):
^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/iritkatriel/src/cpython/Lib/contextlib.py", line 155, in __exit__
self.gen.throw(typ, value, traceback)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/iritkatriel/src/cpython/xx.py", line 116, in maybe_reraise
reraise(e, prefix=prefix, suffix=suffix)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/iritkatriel/src/cpython/xx.py", line 76, in reraise
new_exception = WrappedException(msg)
^^^^^^^^^^^^^^^^^^^^^
TypeError: BaseExceptionGroup.__new__() takes exactly 2 arguments (1 given)
Thanks for pointing this out. Indeed epy.reraise will works with arbitrary __init__ signature. But not arbitrary __new__ signature.
(it would be easy to add if Python was not hardcoding check for built-in exceptions like: TypeError: Exception.__new__(FileNotFoundError) is not safe, use FileNotFoundError.__new__())