astropy / astropy

Astronomy and astrophysics core library
https://www.astropy.org
BSD 3-Clause "New" or "Revised" License
4.37k stars 1.75k forks source link

Use Exceptions with Notes #16879

Open nstarman opened 2 weeks ago

nstarman commented 2 weeks ago

What is the problem this feature will solve?

This is a Meta issue to encourage contributors new and well-seasoned to consider where Astropy can use Exceptions with Notes. One good category is wherever we catch an error, modify the message, and re-raise the same error, e.g. in #16878.

Describe the desired outcome

Please open PRs making our Exception messages more useful!

Additional context

EDIT: Would be much easier to wait for the following bumps first:

This change might also break downstream testing for our exception message with rigid match. Actual impact is unknown.

neutrinoceros commented 2 weeks ago

Exception.add_note is new in Python 3.11 IIRC, and we still support 3.10, so it might be a bit early for PRs, right ?

nstarman commented 2 weeks ago

sys.version_info a la https://github.com/astropy/astropy/pull/16878

pllim commented 2 weeks ago

So, is the ask here to use "notes" or to use "raise from"?

I think I am going to remove "good first issue". We need to actually pinpoint what code can be improved with this ask before asking new contributors to implement. Thanks for your understanding.

pllim commented 2 weeks ago

This might break downstream if they actually look for specific error messages, so we should probably consider the cost/benefit on a case by case basis.

nstarman commented 2 weeks ago

use "notes" or to use "raise from"

The former. Add notes. This shouldn't change the starts of any messages as notes are appended after the error message.

mhvk commented 2 weeks ago

This shouldn't change the starts of any messages as notes are appended after the error message.

Indeed, this is why I liked it so much (even if AttributeError is perhaps not the most logical).

But #16884 made clear that as long as we do not require pytest >= 8.0, one has to be careful, since the notes do not get added to the exception message that pytest sees.

pllim commented 2 weeks ago

Based on the conversation so far, I added stuff to "additional details" section in the original post above. Hope you don't mind!

neutrinoceros commented 2 weeks ago

I forgot about this, but note that we have a helper function astropy.utils.exceptions._add_note_to_exception whose whole purpose is to handle older Python versions transparently.

mhvk commented 2 weeks ago

I forgot about this, but note that we have a helper function astropy.utils.exceptions._add_note_to_exception whose whole purpose is to handle older Python versions transparently.

Yikes! Though hopefully it will soon become irrelevant as we drop python 3.10 -- #16900

neutrinoceros commented 2 weeks ago

Yup, I'm removing it in #16903

pllim commented 2 weeks ago

Just for my own future reference, astropy.utils.exceptions._add_note_to_exception was added in https://github.com/astropy/astropy/pull/16346 that was also milestoned to v7.0