[issue39717] Fix exception causes in tarfile module

[Łukasz Langa]

Change by Łukasz Langa :


--
nosy: +lukasz.langa
nosy_count: 5.0 -> 6.0
pull_requests: +26985
pull_request: https://github.com/python/cpython/pull/28614

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39717] Fix exception causes in tarfile module

[Ethan Furman]

Change by Ethan Furman :


--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39717] Fix exception causes in tarfile module

[Ethan Furman]

Ethan Furman  added the comment:


New changeset b5a6db9111562454617b6771b61f2734ea0420c9 by Ethan Furman in 
branch 'master':
bpo-39717: [tarfile] update nested exception raising (GH-23739)
https://github.com/python/cpython/commit/b5a6db9111562454617b6771b61f2734ea0420c9


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39717] Fix exception causes in tarfile module

[Ethan Furman]

Change by Ethan Furman :


--
versions: +Python 3.10 -Python 3.9

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39717] Fix exception causes in tarfile module

[Ethan Furman]

Change by Ethan Furman :


--
keywords: +patch
pull_requests: +22597
stage:  -> patch review
pull_request: https://github.com/python/cpython/pull/23739

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39717] Fix exception causes in tarfile module

[Ram Rachum]

Ram Rachum  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39717] Fix exception causes in tarfile module

[Ethan Furman]

Ethan Furman  added the comment:

Yes.

Some of the changes are good, others should be `from None`.

The `from None` raises should include the ones where the new exception includes 
the text of the caught exception.

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 `import bz2` at the 
REPL, so `from None` makes sense there.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39717] Fix exception causes in tarfile module

[Ram Rachum]

Ram Rachum  added the comment:

Ethan, got a verdict?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39717] Fix exception causes in tarfile module

[Terry J. Reedy]

Terry J. Reedy  added the comment:

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.

--
nosy: +terry.reedy

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39717] Fix exception causes in tarfile module

[Vedran Čačić]

Vedran Čačić  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39717] Fix exception causes in tarfile module

[Ethan Furman]

Ethan Furman  added the comment:

`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.

PEP3134 did add a lot of justification for your changes, though.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39717] Fix exception causes in tarfile module

[Ram Rachum]

Ram Rachum  added the comment:

Ethan, did we successfully convince you not to use `from None`?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39717] Fix exception causes in tarfile module

[Raymond Hettinger]

Change by Raymond Hettinger :


--
nosy:  -rhettinger

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39717] Fix exception causes in tarfile module

[Ethan Furman]

Ethan Furman  added the comment:

Looking back at PEP3134 [1], the 

raise NewException from exc

is used as one of the examples.


[1] https://www.python.org/dev/peps/pep-3134/#id37

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39717] Fix exception causes in tarfile module

[Vedran Čačić]

Vedran Čačić  added the comment:

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 `raise` under `except` to do that implicitly, maybe there is 
some middle solution. On Python-ideas, recently I saw an idea

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:"
-"During handling of the above exception, another exception occurred:"

I'd be much happier. And it would more often suggest the right thing.

--
nosy: +veky

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39717] Fix exception causes in tarfile module

[Ram Rachum]

Ram Rachum  added the comment:

I'm also strongly against using `from None`. When I'm debugging, I'm like a man 
who got lost in the desert and is about to die of thirst. Any possible insight 
into what happened is like an oasis, even if there are just a few drops of 
water there.

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39717] Fix exception causes in tarfile module

[Martin Panter]

Martin Panter  added the comment:

Please don’t use “from None” in library code. It hides exceptions raised by the 
calling application that would help debugging. E.g. 


--
nosy: +martin.panter

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39717] Fix exception causes in tarfile module

[Ethan Furman]

Ethan Furman  added the comment:

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:

-
"During handling of the above exception, another exception occurred:"

-> there is a bug in the exception handler.
-

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

-> 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 `from 
None` should be preferred; in cases where import errors get translated into 
CompressionErrors those import errors are sometimes useful and sometimes just 
noise -- perhaps including `str(e)` in the new exception is the best way there 
as well.

I'll need to do some research before I decide.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39717] Fix exception causes in tarfile module

[Raymond Hettinger]

Raymond Hettinger  added the comment:

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.

--
assignee:  -> ethan.furman
nosy: +ethan.furman

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39717] Fix exception causes in tarfile module

[Ram Rachum]

Ram Rachum  added the comment:

> What do you think it is necessary to switch from implicit chaining to 
> explicit chaining?

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. 

> If anyone is currently relying on __context__ vs __cause__, this patch will 
> break their code.

I think that the standard you're setting for backward compatibility is 
unreasonably high.

The only code I can think of that inspects `__context__` and `__cause__` is 
code that formats exceptions to be shown to the user, such as Django's debug 
page, Wing IDE's Exceptions pane, whatever's going on in PyCharm etc. That kind 
of code is responsible for checking for both `__context__` and `__cause__` and 
showing the correct message.

If you've seen code outside of the category above that uses `__context__`, I'd 
be curious to see it.

Fortunately, this point is moot since using `raise foo from bar` sets the 
`__context__ = __cause__ = bar`, so the `__context__` will not be changed.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39717] Fix exception causes in tarfile module

[Raymond Hettinger]

Raymond Hettinger  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39717] Fix exception causes in tarfile module

[Raymond Hettinger]

New submission from Raymond Hettinger :

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:

- During handling of the above exception, another exception occurred:
+ The above exception was the direct cause of the following exception:

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.

--
nosy: +rhettinger

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39717] Fix exception causes in tarfile module

[Ram Rachum]

Change by Ram Rachum :


--
components: Library (Lib)
nosy: cool-RR
priority: normal
pull_requests: 17962
severity: normal
status: open
title: Fix exception causes in tarfile module
type: behavior
versions: Python 3.9

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com