New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix exception causes in tarfile module #83898
Comments
What do you think it is necessary to switch from implicit chaining to explicit chaining? If anyone is currently relying on __context__ vs __cause__, this patch will break their code. In a traceback, the only visible difference is in the text between the exceptions:
While we haven't been 100% consistent about this, the norm has been to either use implicit chaining or use "from None" to turn-off chaining. The "from e" approach can be used to alter the explicit chain, perhaps skipping over one or more exceptions, but that isn't the case here. |
If this were new code, there would be a better case for the direct-cause style even though it is more cluttered and a bit slower. I'm only questioning whether it make sense to change it in already deployed code, possibly risking breakage. AFAICT, nothing is currently broken and the minor change in traceback wording would not confer a real benefit. |
I didn't say it's necessary, but I think it's beneficial. I find that message between exceptions useful when I'm debugging and I'd like to keep it accurate in as many places as I can. I think that the more accurate it is, the more people will learn to trust that it has meaning and understand it.
I think that the standard you're setting for backward compatibility is unreasonably high. The only code I can think of that inspects If you've seen code outside of the category above that uses Fortunately, this point is moot since using |
Ethan, would you make the call on this? My recommendation is to close because we usually don't churn APIs unless there is a demonstrable benefit. Also, the current code reflects the dominant practice in the standard library which is both faster and more concise. |
I know we are not in the habit of making large-scale changes to take advantage of new features and enhancements, but I think this may be one of the few exceptions to the rule, and it has to do with what the text between the two tracebacks means: ----- -> there is a bug in the exception handler. ----- -> the first error is the cause and the second error is what we want the user to pay attention to. Whether or not the stdlib is buggy is a pretty big distinction. If this is a change worth making the follow-on question is should we be raising from the previous exception, or from None? How much value is the previous exception adding? In cases where the new exception contains the text of the old one I think I'll need to do some research before I decide. |
Please don’t use “from None” in library code. It hides exceptions raised by the calling application that would help debugging. E.g. <https://bugs.python.org/issue30097#msg293185\> |
I'm also strongly against using Also, some tools like Django and Sentry show you all the local variables for your stacktraces, which is a godsend. These often have important information that sheds light on what went wrong, and if you remove the traceback they'll be gone. |
Oh yes, this has bugged me often. Please fix it somehow. Yes, using "from None" is probably the wrong way to go. Often we need more info in tracebacks, not less. But the "During handling" message is very misleading. Same as Ethan, many times I interpreted it as "something went wrong in the handler" when in fact the handler was doing exactly what it was supposed to do. except WhateverException as e:
raise CustomException from e might be too much to write every time, and while I understand that we cannot simply redefine except WhateverException:
raise as CustomException which I like a lot, but I don't know how hard it is to implement. --- If everything above seems like too much, at least we should consider changing the wording of the message. If it said +"In the handler of the above exception, another exception was raised:" I'd be much happier. And it would more often suggest the right thing. |
Looking back at PEP-3134 [1], the raise NewException from exc is used as one of the examples. |
Ethan, did we successfully convince you not to use |
`Fraid not. It is still going to be a case-by-case basis -- sometimes the previous exception just doesn't add any value, and sometimes it does. PEP-3134 did add a lot of justification for your changes, though. |
I guess https://bugs.python.org/issue39728 is a perfect example of "previous exception not adding any value". :-) And I think it isn't a coincidence that it happens in "your" module. The morale: we think about exceptions in different ways, and it's hard to say what's the right way. Maybe we should just change the wording to imply that __context__ exception didn't spontaneously "occur", but was explicitly "raised" by a handler. |
While I have no specific opinion on tarfile, I strongly disagree with a blanket prohibition on 'from None'. Its proper use is to maintain a defined API and hide irrelevant implementation details. Realistic toy example: def f(x, y):
"Return (x+y)/y for non-zery y."
if y == 0: # Body 1: look ahead.
raise ValueError('y cannot be 0')
else:
return (x+y)/y
# or
try: # Body 2: leap first.
return (x+y)/y
except ZeroDivisionError:
raise ValueError('y cannot be 0') from None 'from e' instead of 'from None' would just add distracting noise. |
Ethan, got a verdict? |
Yes. Some of the changes are good, others should be The What is needed is text captured from the proposed changes to see if the previous exception was useful, and not easily gotten in other ways -- for example, the bz2 ImportError can be easily regenerated by |
I understand. I've closed my PR and I'll let someone else implement this ticket-- I don't want to be the reason that someone didn't get the information they wanted in an error report. Thanks anyway for your time. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: