-
Notifications
You must be signed in to change notification settings - Fork 4
Address feedback from Irit and Guido. #5
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, agreed this is a much better use of GitHub!
I didn't read everything, but here are some comments.
Also, we should try to describe how tracebacks are handled by the various operations on ExceptionGroup (e.g. constructor, split, join).
|
||
```python | ||
try: | ||
raise *(ValueError('a'), ValueError('b'), TypeError('z')) | ||
raise ExceptionGroup(ValueError('a'), ValueError('b'), TypeError('z')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we discuss whether the constructor takes a list of exceptions or uses varargs (*args
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, not yet, I believe. I guess we need to open a new issue to discuss the ExceptionGroup API in detail and later fold it into this proposal. @iritkatriel?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think varargs is nice. In reality I think you would want to create it empty and add exceptions to it. But whichever way it's constructed - what happens when you try to raise an empty ExceptionGroup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think varargs is nice. In reality I think you would want to create it empty and add exceptions to it. But whichever way it's constructed - what happens when you try to raise an empty ExceptionGroup?
I've created an issue to discuss that: #6
except_star.md
Outdated
# would crash with: | ||
# | ||
# ExceptionGroup( | ||
# ValueError('a'), | ||
# ExceptionGroup( | ||
# TypeError('b'), | ||
# TypeError('c'), | ||
# ), | ||
# ExceptionGroup( | ||
# KeyError('d') | ||
# ) | ||
# ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yikes! But why would it keep that group? If you start with ExceptionGroup(A(), B())
and you write
except *B as e:
raise e
I hope you wouldn't end up with ExceptionGroup(A(), ExceptionGroup(B()))
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hoped you would comment on this! I feel like this is one of the trickiest behaviors for us to define.
If you start with ExceptionGroup(A(), B()) and you write
except *B as e: raise e
I hope you wouldn't end up with ExceptionGroup(A(), ExceptionGroup(B()))?
I think it should end up with ExceptionGroup(A(), ExceptionGroup(B()))
. What if we change your code to:
except *B as e:
e.marker = 'spam'
raise e
I would expect the marker='spam'
attribute to be present somewhere in what propagates out of the try
block. And if we merge e
back and raise ExceptionGroup(A(), B())
in the end this attribute will be lost.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm... But should you be doing that? In a sense ‘e’ is ephemeral, only for use in this one block. Surely if you mutate ‘e’ and then use bare raise it should get lost? (Hm, how can we describe bare raise? Ideally it should push the exceptions extracted back exactly from where they were extracted. Maybe the EG API needs to be described more carefully.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Surely if you mutate ‘e’ and then use bare raise it should get lost?
That's a good point. So you propose that raise e
would be equivalent to raise
. If the user wants to "flatten" the hierarchy that would explicitly say raise ExceptionGroup(*e)
.
I like this idea because it makes it harder for users to accidentally flatten parts of their exceptions tree. The current wording was motivated by the e.marker = 'spam'
example, but if we treat e
as an ephemeral object we can perhaps ignore this use case.
This is also an area where a linter (or mypy, easily) can warn the user that they're working with an ephemeral object and that their changes or references to it will not persist the way they expect them to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've pushed another update erasing any semantic difference between raise
and raise e
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, you mean that bare raise doesn't update the traceback, whereas raise e
would add that very "raise e" line to the traceback, right? Or is there something about e.__context__
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guido, can you explain re __context__
? I don't see any difference between raise
and raise e
here:
def foo():
try:
1 / 0
except ZeroDivisionError:
try:
raise ValueError
except Exception as e:
raise e
try:
foo()
except Exception as e:
print(type(e), type(e.__context__))
# always prints "<class 'ValueError'> <class 'ZeroDivisionError'>"
or here:
def foo():
try:
1 / 0
except ZeroDivisionError as e:
raise e
try:
foo()
except Exception as e:
print(type(e), type(e.__context__))
# always prints "<class 'ZeroDivisionError'> <class 'NoneType'>"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not __context__
but the traceback that's different. Compare:
(v39) ~$ cat -n a.py; python a.py
1 def foo():
2 try:
3 1/0
4 except ZeroDivisionError as e:
5 raise e
6
7 foo()
Traceback (most recent call last):
File "/Users/guido/a.py", line 7, in <module>
foo()
File "/Users/guido/a.py", line 5, in foo
raise e
File "/Users/guido/a.py", line 3, in foo
1/0
ZeroDivisionError: division by zero
to
(v39) ~$ cat -n b.py; python b.py
1 def foo():
2 try:
3 1/0
4 except ZeroDivisionError as e:
5 raise
6
7 foo()
Traceback (most recent call last):
File "/Users/guido/b.py", line 7, in <module>
foo()
File "/Users/guido/b.py", line 3, in foo
1/0
ZeroDivisionError: division by zero
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, i'll push a commit clarifying this soon.
except_star.md
Outdated
We propose to replicate this behavior in the `except*` syntax as it is useful | ||
as an escape hatch when it's clear that all exceptions can be silenced. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless you only allow return
without expression that's not enough -- what would happen here?
try:
raise ExceptionGroup(A(), B())
except *A:
return 1
except *B:
return 2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, great catch. I think we should only allow bare return. I'll add a clarification.
(FWIW I never use this feature myself)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what would happen here?
I think it would return 1
, because A()
would be matched first. But when running async code this is too fragile and would only allow users to write unpredictable code. IMO we should disable non-None returns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There’s room to disagree here, but I would prefer not allowing any return statements over only allowing bare return. I worry that allowing bare return with the proposed semantics is closing the door for assigning any semantics to return with expression and for break/continue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but I would prefer not allowing any return statements over only allowing bare return
TBH I'd prefer that too. As I said earlier I myself don't use this feature. Relying on a single "return" buried in finally
or some except
smells like bad code to me. But I'm also not sure how popular this idiom is: I can only recall seeing it once or twice in the wild.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just pushed an update prohibiting returns in except* altogether.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 610b4db
We absolutely should. @iritkatriel would you like to contribute a section? Trio's |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I committed some minor edits. In addition:
-
The two code snippets under "Raising an ExceptionGroup introduces nesting:" should come after the "Unmatched exceptions" section because they don't quite make sense yet where they are.
-
around line 205: "(with the group of unprocessed exceptions referenced via the context attribute.)" We are now in the middle of processing an exception group, and this made me pause and ask "unprocessed at which point in time?" This needs to be made more precise, probably with pseudo code that covers all cases.
-
I'll try to write up the section on tracebacks.
Awesome, thanks!
I've reordered the sections.
I've updated the language there. |
@iritkatriel @gvanrossum thanks for the thorough reviews! I've merged this pr. Here's the rendered document: https://github.com/python/exceptiongroups/blob/master/except_star.md |
No description provided.