Fix exception causes in package.py

[cool-RR]

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!

[bhrutledge]

Makes sense to me. That said, can you provide a link to the Python docs or an article/guide that recommends this change?

Also, one nitpick on the variable name before you proceed.

[bhrutledge]

Thus far, we've used exc for the exception:

Suggested change

	except FileNotFoundError as e:
	except FileNotFoundError as exc:

[cool-RR]

Will do.

[cool-RR]

Regarding documentation, there's this: https://www.python.org/dev/peps/pep-0344/#enhanced-reporting

[sigmavirus24]

@bhrutledge correct me if I'm wrong but we don't print stack traces as a result of these exceptions. This seems like work with no end-user value.

[bhrutledge]

we don't print stack traces as a result of these exceptions.

Good point; they're captured in __main__:

twine/twine/__main__.py

Lines 24 to 28 in f922eeb

	def main() -> Any:
	try:
	return cli.dispatch(sys.argv[1:])
	except (exceptions.TwineException, requests.HTTPError) as exc:
	return "{}: {}".format(exc.__class__.__name__, exc.args[0])

This seems like work with no end-user value.

Except maybe for people who use Twine as a library? Also, this reminds me that we've talked recently in #586 (comment) about adding the traceback to --verbose output.

So, since this seems harmless, and if @cool-RR is still motivated to do it, I'm game to review it.

One note: looking over other projects where @cool-RR has done this, a common suggestion is to use from None to suppress the underlying "noisy" exception, which @cool-RR is opposed to (for example: pytest-dev/pytest#7351 (comment)). Since the errors aren't user facing at the moment, I'm not worried about it, and I think it's an easy thing to change later.

Also, I appreciate the education in exception handling.

[sigmavirus24]

Except maybe for people who use Twine as a library?

I think almost all of those people are handling exceptions appropriately. None have reported this as a problem. I'd be wary of saying "This seems harmless because it might help people who ostensibly don't have a problem yet?".

Since the errors aren't user facing at the moment, I'm not worried about it, and I think it's an easy thing to change later.

Frankly, if we're not benefiting someone here (ourselves, end-users, or API users) I'm not sure that we should maintain this regardless of whether it's harmless.

If we want to spin this for API users, I think we still need to pump the breaks and think about what that looks like without just merging this. What do we want exception handling to look like for an API user? What do we want them to see and have for debugging? Should we always use from None and encapsulate the original exception into the new one so it's discoverable? Just because this particular contributor is opposed to something doesn't make it the wrong choice for us and the design we're trying to achieve.

[bhrutledge]

if we're not benefiting someone here (ourselves, end-users, or API users) I'm not sure that we should maintain this regardless of whether it's harmless.

Agreed. I'm going to close this, and we revisit it if necessary.

@cool-RR Thanks for proposal, and keeping it minimal. Good luck on your mission. 😉

[sigmavirus24]

Woops, definitely meant "merge" not "maintain".

Thanks for talking through this with me @bhrutledge

[cool-RR]

I understand. Thanks for your time and attention.